Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
TREE_SEARCH ioctl doesn't help in this case the dev item don't provide all the info that btrfs-progs might need in the long run (and even the current needs). Thanks, Anand On 06/11/2013 10:24 PM, Josef Bacik wrote: On Mon, Jun 10, 2013 at 08:59:15AM -0600, Anand Jain wrote: This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS which reads the btrfs_fs_devices and btrfs_device structure from the kernel respectively. The information in these structure are useful to report the device/fs information in line with the kernel operations and thus immediately addresses the problem that 'btrfs fi show' command reports the stale information after device device add remove operation is performed. That is because btrfs fi show reads the disks directly. Further the frame-work provided here would help to enhance the btrfs-progs/library to read the other fs information and its device information. Also the frame work provided here is easily extensible to retrieve any other structure as future needs. v1-v2: .code optimized .get the device generation number as well, so that btrfs-progs could print using print_one_uuid Signed-off-by: Anand Jain anand.j...@oracle.com In fact NACK altogether on this patch, you can get the same info out with the TREE_SEARCH ioctl, just do that in btrfs-progs and don't add yet another ioctl. Thanks, Josef -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On 06/11/2013 03:40 AM, Josef Bacik wrote: On Mon, Jun 10, 2013 at 08:59:15AM -0600, Anand Jain wrote: This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS which reads the btrfs_fs_devices and btrfs_device structure from the kernel respectively. The information in these structure are useful to report the device/fs information in line with the kernel operations and thus immediately addresses the problem that 'btrfs fi show' command reports the stale information after device device add remove operation is performed. That is because btrfs fi show reads the disks directly. Further the frame-work provided here would help to enhance the btrfs-progs/library to read the other fs information and its device information. Also the frame work provided here is easily extensible to retrieve any other structure as future needs. Please submit an xfstest along with this to test the new functionality so we have something to test it with. Make sure it runs properly if your patches are not in place since they obviously won't be for most people. Once you have an xfstest I can properly test these patches. Thanks, This kernel patch supports a new cli option --kernel under the existing command 'btrfs filesystem show'. xfstest first of all would need testcase to test the btrfs filesystem show which isn't there yet. Doing it here deviate too much from the point here. But let me try to quick get that. Thanks, Anand -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On Tue, Jun 11, 2013 at 07:10:12AM -0600, anand jain wrote: On 06/11/2013 03:40 AM, Josef Bacik wrote: On Mon, Jun 10, 2013 at 08:59:15AM -0600, Anand Jain wrote: This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS which reads the btrfs_fs_devices and btrfs_device structure from the kernel respectively. The information in these structure are useful to report the device/fs information in line with the kernel operations and thus immediately addresses the problem that 'btrfs fi show' command reports the stale information after device device add remove operation is performed. That is because btrfs fi show reads the disks directly. Further the frame-work provided here would help to enhance the btrfs-progs/library to read the other fs information and its device information. Also the frame work provided here is easily extensible to retrieve any other structure as future needs. Please submit an xfstest along with this to test the new functionality so we have something to test it with. Make sure it runs properly if your patches are not in place since they obviously won't be for most people. Once you have an xfstest I can properly test these patches. Thanks, This kernel patch supports a new cli option --kernel under the existing command 'btrfs filesystem show'. xfstest first of all would need testcase to test the btrfs filesystem show which isn't there yet. Doing it here deviate too much from the point here. But let me try to quick get that. No you just need a testcase to make sure --kernel is working properly, so make it use SCRATCH_DEV_POOL and do things like make a file system, remove a device, make sure its no longer there, add it back and make sure it pops up again, that sort of thing. We are trying to make btrfs more stable and I have no interest in taking in new features/ioctls without a way to test them properly, so it is not beside the point. Thanks, Josef -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On 06/11/2013 04:30 AM, Zach Brown wrote:
On Mon, Jun 10, 2013 at 10:59:15PM +0800, Anand Jain wrote:
This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS
which reads the btrfs_fs_devices and btrfs_device structure
from the kernel respectively.
Why not just use sysfs to export the device lists?
Hmm. I see sysfs being used but isn't ioctl more easy
to get stuff out from the kernel (except for dumping
address_space) ? Providing a complete sysfs interface
for btrfs can be a RFC as well. For now ioctl will help
to fix the bugs.
The information in these structure are useful to report the
device/fs information in line with the kernel operations and
thus immediately addresses the problem that 'btrfs fi show'
command reports the stale information after device device add
remove operation is performed. That is because btrfs fi show
reads the disks directly.
Hmm. Would it be enough to get the block device address_space in sync
with the btrfs stuctures? Would that get rid of the need for this
interface?
Absolutely ! I have that to experiment in linux for
a long time. More to come on that as time permits from
the bug fixes which is kind of urgent need as of now.
But sure, for the sake of argument and code review, let's say that we
wanted some ioctls to read the btrfs kernel device lists.
thanks.
Further the frame-work provided here would help to enhance
Please don't add a layer of generic encoding above these new ioctls.
It's not necessary and it makes more work for the various parts of the
world that want to do deep ioctl inspection (think, for example, of
strace).
Agreed. Debugging has to be easier.
+static size_t get_ioctl_sz(size_t pl_list_sz, u64 mul, u64 alloc_cnt)
+
+static int get_ioctl_header_and_payload(unsigned long arg,
+ size_t unit_sz, int default_len,
+ struct btrfs_ioctl_header **argp, size_t *sz)
This adds a lot of fiddly math and allocation that can go wrong for no
benefit. We can restructure the interface so all of this code goes
away.
1) Have a simple single fixed input structure for each ioctl. Maybe
with some extra padding and a flags argument if you think stuff is going
to be added over time. No generic header. No casting. The ioctl
number defines the input structure. If you need a different structure
later, use a different ioctl number.
-No generic header and No casting- Why not ? anything that
might not work with strace ?
(Header-payload model are typically favorites of IO protocol drivers.
they are easily extensible, checking for compatibility is easier, and
code re-useability is great).
Thanks for the review comments below. I will look into that.
Anand
---
2) Don't have a fixed array of output structs embedded in the input
struct. Have a pointer to the array of output structs in the input
struct. Probably with a number of elements found there.
3) Don't copy the giant user output buffer into the kernel, write to it,
then copy it back to user space. Instead have the kernel copy_to_user()
each output structure to the userspace pointer as it goes. Have the
ioctl() return a positive count of the number of elements copied.
static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct btrfs_ioctl_vol_args *vol;
+ struct btrfs_ioctl_vol_args *vol = NULL;
struct btrfs_fs_devices *fs_devices;
- int ret = -ENOTTY;
+ struct btrfs_ioctl_header *argp = NULL;
+ int ret = -ENOTTY;
+ size_t sz = 0;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- vol = memdup_user((void __user *)arg, sizeof(*vol));
- if (IS_ERR(vol))
- return PTR_ERR(vol);
-
switch (cmd) {
case BTRFS_IOC_SCAN_DEV:
+ vol = memdup_user((void __user *)arg, sizeof(*vol));
+ if (IS_ERR(vol))
+ return PTR_ERR(vol);
Hmm, having to duplicate that previously shared setup into each ioctl
block is a sign that the layering is wrong.
Don't smush the new ioctls into case bodies of this existing simple
fuction that worked with the _vol_args ioctls. Rename this
btrfs_ioctl_vol_args, or something, and call it from a tiny new
btrfs_control_ioctl() for its two ioctls. That new high level btrfs
ioctl dispatch function can then call the two new _get_devs and
_get_fsids functions.
+ case BTRFS_IOC_GET_FSIDS:
+ ret = get_ioctl_header_and_payload(arg,
+ sizeof(struct btrfs_ioctl_fs_list),
+ BTRFS_FSIDS_LEN, argp, sz);
+ if (ret == 1) {
+ ret = copy_to_user((void __user *)arg, argp, sz);
+ kfree(argp);
+ return -EFAULT;
+ } else if (ret)
+ return -EFAULT;
+ ret = btrfs_get_fsids(argp);
+ if
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On Mon, Jun 10, 2013 at 08:59:15AM -0600, Anand Jain wrote: This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS which reads the btrfs_fs_devices and btrfs_device structure from the kernel respectively. The information in these structure are useful to report the device/fs information in line with the kernel operations and thus immediately addresses the problem that 'btrfs fi show' command reports the stale information after device device add remove operation is performed. That is because btrfs fi show reads the disks directly. Further the frame-work provided here would help to enhance the btrfs-progs/library to read the other fs information and its device information. Also the frame work provided here is easily extensible to retrieve any other structure as future needs. v1-v2: .code optimized .get the device generation number as well, so that btrfs-progs could print using print_one_uuid Signed-off-by: Anand Jain anand.j...@oracle.com In fact NACK altogether on this patch, you can get the same info out with the TREE_SEARCH ioctl, just do that in btrfs-progs and don't add yet another ioctl. Thanks, Josef -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
1) Have a simple single fixed input structure for each ioctl. Maybe with some extra padding and a flags argument if you think stuff is going to be added over time. No generic header. No casting. The ioctl number defines the input structure. If you need a different structure later, use a different ioctl number. -No generic header and No casting- Why not ? Because it brings no benefits. All the supposed benefits can be achived through careful use of the existing interfaces, ioctl or otherwise. And it has costs: code complexity, run-time cpu/memory, etc. Its benefit/cost ratio is not flattering :). - z -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On Mon, Jun 10, 2013 at 08:59:15AM -0600, Anand Jain wrote: This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS which reads the btrfs_fs_devices and btrfs_device structure from the kernel respectively. The information in these structure are useful to report the device/fs information in line with the kernel operations and thus immediately addresses the problem that 'btrfs fi show' command reports the stale information after device device add remove operation is performed. That is because btrfs fi show reads the disks directly. Further the frame-work provided here would help to enhance the btrfs-progs/library to read the other fs information and its device information. Also the frame work provided here is easily extensible to retrieve any other structure as future needs. Please submit an xfstest along with this to test the new functionality so we have something to test it with. Make sure it runs properly if your patches are not in place since they obviously won't be for most people. Once you have an xfstest I can properly test these patches. Thanks, Josef -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2 v2] btrfs: add framework to read fs info and dev info from the kernel
On Mon, Jun 10, 2013 at 10:59:15PM +0800, Anand Jain wrote:
This adds two ioctl BTRFS_IOC_GET_FSIDS and BTRFS_IOC_GET_DEVS
which reads the btrfs_fs_devices and btrfs_device structure
from the kernel respectively.
Why not just use sysfs to export the device lists?
The information in these structure are useful to report the
device/fs information in line with the kernel operations and
thus immediately addresses the problem that 'btrfs fi show'
command reports the stale information after device device add
remove operation is performed. That is because btrfs fi show
reads the disks directly.
Hmm. Would it be enough to get the block device address_space in sync
with the btrfs stuctures? Would that get rid of the need for this
interface?
But sure, for the sake of argument and code review, let's say that we
wanted some ioctls to read the btrfs kernel device lists.
Further the frame-work provided here would help to enhance
Please don't add a layer of generic encoding above these new ioctls.
It's not necessary and it makes more work for the various parts of the
world that want to do deep ioctl inspection (think, for example, of
strace).
+static size_t get_ioctl_sz(size_t pl_list_sz, u64 mul, u64 alloc_cnt)
+
+static int get_ioctl_header_and_payload(unsigned long arg,
+ size_t unit_sz, int default_len,
+ struct btrfs_ioctl_header **argp, size_t *sz)
This adds a lot of fiddly math and allocation that can go wrong for no
benefit. We can restructure the interface so all of this code goes
away.
1) Have a simple single fixed input structure for each ioctl. Maybe
with some extra padding and a flags argument if you think stuff is going
to be added over time. No generic header. No casting. The ioctl
number defines the input structure. If you need a different structure
later, use a different ioctl number.
2) Don't have a fixed array of output structs embedded in the input
struct. Have a pointer to the array of output structs in the input
struct. Probably with a number of elements found there.
3) Don't copy the giant user output buffer into the kernel, write to it,
then copy it back to user space. Instead have the kernel copy_to_user()
each output structure to the userspace pointer as it goes. Have the
ioctl() return a positive count of the number of elements copied.
static long btrfs_control_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
- struct btrfs_ioctl_vol_args *vol;
+ struct btrfs_ioctl_vol_args *vol = NULL;
struct btrfs_fs_devices *fs_devices;
- int ret = -ENOTTY;
+ struct btrfs_ioctl_header *argp = NULL;
+ int ret = -ENOTTY;
+ size_t sz = 0;
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
- vol = memdup_user((void __user *)arg, sizeof(*vol));
- if (IS_ERR(vol))
- return PTR_ERR(vol);
-
switch (cmd) {
case BTRFS_IOC_SCAN_DEV:
+ vol = memdup_user((void __user *)arg, sizeof(*vol));
+ if (IS_ERR(vol))
+ return PTR_ERR(vol);
Hmm, having to duplicate that previously shared setup into each ioctl
block is a sign that the layering is wrong.
Don't smush the new ioctls into case bodies of this existing simple
fuction that worked with the _vol_args ioctls. Rename this
btrfs_ioctl_vol_args, or something, and call it from a tiny new
btrfs_control_ioctl() for its two ioctls. That new high level btrfs
ioctl dispatch function can then call the two new _get_devs and
_get_fsids functions.
+ case BTRFS_IOC_GET_FSIDS:
+ ret = get_ioctl_header_and_payload(arg,
+ sizeof(struct btrfs_ioctl_fs_list),
+ BTRFS_FSIDS_LEN, argp, sz);
+ if (ret == 1) {
+ ret = copy_to_user((void __user *)arg, argp, sz);
+ kfree(argp);
+ return -EFAULT;
+ } else if (ret)
+ return -EFAULT;
+ ret = btrfs_get_fsids(argp);
+ if (ret == 0 copy_to_user((void __user *)arg, argp, sz))
+ ret = -EFAULT;
+ kfree(argp);
+ break;
All of this can go away if you have trivial input and array-of-output
args that the ioctl helper works with.
case BTRFS_IOC_GET_FSIDS:
ret = btrfs_ioctl_get_fsids(arg);
break;
+int btrfs_get_fsids(struct btrfs_ioctl_header *argp)
+{
+ u64 cnt = 0;
+ struct btrfs_device *device;
+ struct btrfs_fs_devices *fs_devices;
+ struct btrfs_ioctl_fs_list *fslist;
+ struct btrfs_ioctl_fsinfo *fsinfo;
+
+ fslist = (struct btrfs_ioctl_fs_list *)((u8 *)argp
+ + sizeof(*argp));
Instead of working with an allocated copy of the input, copy the user
input struct onto the stack here.
struct btrfs_ioctl_get_fsids_input in;
