| Message ID | 1408351099-4721-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote: > ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed > device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed > disk when probed. So in the userland we hit a count-slot missmatch > bug.. > get_fs_info() > :: > BUG_ON(ndevs >= fi_args->num_devices); > which hits this bug when we have mounted a seed device. > > So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO > will provide total_devices instead of num_devices. The ioctl is very unclear what the 'num_device' actually means. > This would fix the problem partly. Partly because ealier num_devices > included the replacing device but now total_device does not include > the replacing device. Getting a count which includes a transient device > is rather too in efficient/wrong indeed, because there can be a race > condition where in the time between ioctl BTRFS_IOC_FS_INFO to > BTRFS_IOC_DEV_INFO the replace device operation might have been > completed. So to fix this problem its better that user land btrfs-progs > probes replacing device (at devid 0) separately. > > v2: > Agree with Wang's comment. Its better to show seed disks under the > sprout fs, so that user can establish mapping of seed to sprout devices. > > So here I am making BTRFS_IOC_FS_INFO to return the total_devices > which would count the seed devices (but not the replacing device). This is even more confusing. I think we need to add another member to the ioctl struct to reflect the number of regular devices (num_devices) and the true total number of devices including seeding and replaced devices. The difference should be accompanied by a flag that would say if there's a seeding or replace in progress. There are some backward compatibility concerns. Setting num_devices to total_devices changes semantics of the ioctl, so I think it should stay as is for now, but the BUG_ON can be removed and replaced by code that reallocates the buffer or allocates a few more items in advance. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/04/2014 05:58 PM, David Sterba wrote: > On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote: >> ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed >> device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed >> disk when probed. So in the userland we hit a count-slot missmatch >> bug.. >> get_fs_info() >> :: >> BUG_ON(ndevs >= fi_args->num_devices); >> which hits this bug when we have mounted a seed device. >> >> So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO >> will provide total_devices instead of num_devices. > > The ioctl is very unclear what the 'num_device' actually means. Right. Thats also true in kernel. very messy. very confusing. tool btrfs-devlist would help understand whats going on. ---- $ egrep num_device *.c | egrep "total_device" ioctl.c: fi_args->num_devices = fs_devices->total_devices; super.c: ret = !(fs_devices->num_devices == fs_devices->total_devices); volumes.c: total_devices = btrfs_super_num_devices(disk_super); ---- By the way about BTRFS_IOC_DEVICES_READY ioctl above its long time broken with seed/replace, just waiting to get these patches integrated first so to fix it later. >> This would fix the problem partly. Partly because ealier num_devices >> included the replacing device but now total_device does not include >> the replacing device. Getting a count which includes a transient device >> is rather too in efficient/wrong indeed, because there can be a race >> condition where in the time between ioctl BTRFS_IOC_FS_INFO to >> BTRFS_IOC_DEV_INFO the replace device operation might have been >> completed. So to fix this problem its better that user land btrfs-progs >> probes replacing device (at devid 0) separately. >> >> v2: >> Agree with Wang's comment. Its better to show seed disks under the >> sprout fs, so that user can establish mapping of seed to sprout devices. >> >> So here I am making BTRFS_IOC_FS_INFO to return the total_devices >> which would count the seed devices (but not the replacing device). > > This is even more confusing. I think we need to add another member to > the ioctl struct to reflect the number of regular devices (num_devices) > and the true total number of devices including seeding and replaced > devices. that will be a better way. thanks. > The difference should be accompanied by a flag that would say > if there's a seeding or replace in progress. > > There are some backward compatibility concerns. Setting num_devices to > total_devices changes semantics of the ioctl, so I think it should stay > as is for now, As I have tested there is not backward compatibility issue. But from semantics perspective .. agreed. > but the BUG_ON can be removed and replaced by code that > reallocates the buffer or allocates a few more items in advance. We don't know how may seed devices are there for a sprout FS. So thats not possible. Will review resubmit. Thanks for commenting. Anand > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-09-04 at 20:02 +0800, Anand Jain wrote: > > > On 09/04/2014 05:58 PM, David Sterba wrote: > > On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote: > >> ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed > >> device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed > >> disk when probed. So in the userland we hit a count-slot missmatch > >> bug.. > >> get_fs_info() > >> :: > >> BUG_ON(ndevs >= fi_args->num_devices); > >> which hits this bug when we have mounted a seed device. > >> > >> So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO > >> will provide total_devices instead of num_devices. > > > > The ioctl is very unclear what the 'num_device' actually means. > > Right. Thats also true in kernel. very messy. very confusing. > tool btrfs-devlist would help understand whats going on. > > ---- > $ egrep num_device *.c | egrep "total_device" > ioctl.c: fi_args->num_devices = fs_devices->total_devices; > super.c: ret = !(fs_devices->num_devices == fs_devices->total_devices); > volumes.c: total_devices = btrfs_super_num_devices(disk_super); > ---- > > By the way about BTRFS_IOC_DEVICES_READY ioctl above its long time > broken with seed/replace, just waiting to get these patches integrated > first so to fix it later. > > > >> This would fix the problem partly. Partly because ealier num_devices > >> included the replacing device but now total_device does not include > >> the replacing device. Getting a count which includes a transient device > >> is rather too in efficient/wrong indeed, because there can be a race > >> condition where in the time between ioctl BTRFS_IOC_FS_INFO to > >> BTRFS_IOC_DEV_INFO the replace device operation might have been > >> completed. So to fix this problem its better that user land btrfs-progs > >> probes replacing device (at devid 0) separately. > >> > >> v2: > >> Agree with Wang's comment. Its better to show seed disks under the > >> sprout fs, so that user can establish mapping of seed to sprout devices. > >> > >> So here I am making BTRFS_IOC_FS_INFO to return the total_devices > >> which would count the seed devices (but not the replacing device). > > > > This is even more confusing. I think we need to add another member to > > the ioctl struct to reflect the number of regular devices (num_devices) > > and the true total number of devices including seeding and replaced > > devices. > > that will be a better way. thanks. > > > The difference should be accompanied by a flag that would say > > if there's a seeding or replace in progress. > > > > There are some backward compatibility concerns. Setting num_devices to > > total_devices changes semantics of the ioctl, so I think it should stay > > as is for now, > > As I have tested there is not backward compatibility issue. > But from semantics perspective .. agreed. > > > but the BUG_ON can be removed and replaced by code that > > reallocates the buffer or allocates a few more items in advance. > > We don't know how may seed devices are there for a sprout FS. > So thats not possible. > > Will review resubmit. > > Thanks for commenting. Hi all, Firtly, thanks for the fix, Anand, how's the new version going? I've been testing the btrfs fi show cmd these days and find that this patch has not been merged into linus's tree yet. Since the suggested way of adding member to the ioctl struct brings compatibility issues, it may need more discussion. But since this fix really incluence much to the user, I consider merging this version first to be a good idea. What do you think, Chris? Thanks, Gui > Anand > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
inline as below. On 10/17/2014 09:58 AM, Gui Hecheng wrote: > On Thu, 2014-09-04 at 20:02 +0800, Anand Jain wrote: >> >> >> On 09/04/2014 05:58 PM, David Sterba wrote: >>> On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote: >>>> ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed >>>> device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed >>>> disk when probed. So in the userland we hit a count-slot missmatch >>>> bug.. >>>> get_fs_info() >>>> :: >>>> BUG_ON(ndevs >= fi_args->num_devices); >>>> which hits this bug when we have mounted a seed device. >>>> >>>> So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO >>>> will provide total_devices instead of num_devices. >>> >>> The ioctl is very unclear what the 'num_device' actually means. >> >> Right. Thats also true in kernel. very messy. very confusing. >> tool btrfs-devlist would help understand whats going on. >> >> ---- >> $ egrep num_device *.c | egrep "total_device" >> ioctl.c: fi_args->num_devices = fs_devices->total_devices; >> super.c: ret = !(fs_devices->num_devices == fs_devices->total_devices); >> volumes.c: total_devices = btrfs_super_num_devices(disk_super); >> ---- >> >> By the way about BTRFS_IOC_DEVICES_READY ioctl above its long time >> broken with seed/replace, just waiting to get these patches integrated >> first so to fix it later. >> >> >>>> This would fix the problem partly. Partly because ealier num_devices >>>> included the replacing device but now total_device does not include >>>> the replacing device. Getting a count which includes a transient device >>>> is rather too in efficient/wrong indeed, because there can be a race >>>> condition where in the time between ioctl BTRFS_IOC_FS_INFO to >>>> BTRFS_IOC_DEV_INFO the replace device operation might have been >>>> completed. So to fix this problem its better that user land btrfs-progs >>>> probes replacing device (at devid 0) separately. >>>> >>>> v2: >>>> Agree with Wang's comment. Its better to show seed disks under the >>>> sprout fs, so that user can establish mapping of seed to sprout devices. >>>> >>>> So here I am making BTRFS_IOC_FS_INFO to return the total_devices >>>> which would count the seed devices (but not the replacing device). >>> >>> This is even more confusing. I think we need to add another member to >>> the ioctl struct to reflect the number of regular devices (num_devices) >>> and the true total number of devices including seeding and replaced >>> devices. >> >> that will be a better way. thanks. >> >>> The difference should be accompanied by a flag that would say >>> if there's a seeding or replace in progress. >>> >>> There are some backward compatibility concerns. Setting num_devices to >>> total_devices changes semantics of the ioctl, so I think it should stay >>> as is for now, >> >> As I have tested there is not backward compatibility issue. >> But from semantics perspective .. agreed. >> >>> but the BUG_ON can be removed and replaced by code that >>> reallocates the buffer or allocates a few more items in advance. >> >> We don't know how may seed devices are there for a sprout FS. >> So thats not possible. >> >> Will review resubmit. >> >> Thanks for commenting. > > Hi all, > > Firtly, thanks for the fix, Anand, how's the new version going? > I've been testing the btrfs fi show cmd these days and find that > this patch has not been merged into linus's tree yet. > > Since the suggested way of adding member to the ioctl struct brings > compatibility issues, it may need more discussion. Thanks Gui for commenting. Yes discussion is needed. Mainly on our long term plan for a better btrfs kernel device and parameter read interface. which also means current bugs can wait for this new interface instead of patching the old structures and bring in a new compatibility mess. Thanks, Anand > But since this fix really incluence much to the user, I consider merging > this version first to be a good idea. > > What do you think, Chris? > > Thanks, > Gui > >> Anand >> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
There will be compatibility issue with this patch running older kernel, sorry I slipped some combination. As I see this is already in, I am sending a patch to back out this changes if it helps. Thanks. On 09/04/14 20:02, Anand Jain wrote: > > > > On 09/04/2014 05:58 PM, David Sterba wrote: >> On Mon, Aug 18, 2014 at 04:38:18PM +0800, Anand Jain wrote: >>> ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed >>> device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed >>> disk when probed. So in the userland we hit a count-slot missmatch >>> bug.. >>> get_fs_info() >>> :: >>> BUG_ON(ndevs >= fi_args->num_devices); >>> which hits this bug when we have mounted a seed device. >>> >>> So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO >>> will provide total_devices instead of num_devices. >> >> The ioctl is very unclear what the 'num_device' actually means. > > Right. Thats also true in kernel. very messy. very confusing. > tool btrfs-devlist would help understand whats going on. > > ---- > $ egrep num_device *.c | egrep "total_device" > ioctl.c: fi_args->num_devices = fs_devices->total_devices; > super.c: ret = !(fs_devices->num_devices == > fs_devices->total_devices); > volumes.c: total_devices = btrfs_super_num_devices(disk_super); > ---- > > By the way about BTRFS_IOC_DEVICES_READY ioctl above its long time > broken with seed/replace, just waiting to get these patches integrated > first so to fix it later. > > >>> This would fix the problem partly. Partly because ealier num_devices >>> included the replacing device but now total_device does not include >>> the replacing device. Getting a count which includes a transient device >>> is rather too in efficient/wrong indeed, because there can be a race >>> condition where in the time between ioctl BTRFS_IOC_FS_INFO to >>> BTRFS_IOC_DEV_INFO the replace device operation might have been >>> completed. So to fix this problem its better that user land btrfs-progs >>> probes replacing device (at devid 0) separately. >>> >>> v2: >>> Agree with Wang's comment. Its better to show seed disks under the >>> sprout fs, so that user can establish mapping of seed to sprout devices. >>> >>> So here I am making BTRFS_IOC_FS_INFO to return the total_devices >>> which would count the seed devices (but not the replacing device). >> >> This is even more confusing. I think we need to add another member to >> the ioctl struct to reflect the number of regular devices (num_devices) >> and the true total number of devices including seeding and replaced >> devices. > > that will be a better way. thanks. > >> The difference should be accompanied by a flag that would say >> if there's a seeding or replace in progress. >> >> There are some backward compatibility concerns. Setting num_devices to >> total_devices changes semantics of the ioctl, so I think it should stay >> as is for now, > > As I have tested there is not backward compatibility issue. > But from semantics perspective .. agreed. > >> but the BUG_ON can be removed and replaced by code that >> reallocates the buffer or allocates a few more items in advance. > > We don't know how may seed devices are there for a sprout FS. > So thats not possible. > > Will review resubmit. > > Thanks for commenting. > > Anand > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index da73ab3..05dd88b 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2725,7 +2725,7 @@ static long btrfs_ioctl_fs_info(struct btrfs_root *root, void __user *arg) return -ENOMEM; mutex_lock(&fs_devices->device_list_mutex); - fi_args->num_devices = fs_devices->num_devices; + fi_args->num_devices = fs_devices->total_devices; memcpy(&fi_args->fsid, root->fs_info->fsid, sizeof(fi_args->fsid)); list_for_each_entry_safe(device, next, &fs_devices->devices, dev_list) {
ioctl BTRFS_IOC_FS_INFO return num_devices which does _not_ include seed device, But the following ioctl BTRFS_IOC_DEV_INFO counts and gets seed disk when probed. So in the userland we hit a count-slot missmatch bug.. get_fs_info() :: BUG_ON(ndevs >= fi_args->num_devices); which hits this bug when we have mounted a seed device. So to fix this problem here in this patch ioctl BTRFS_IOC_FS_INFO will provide total_devices instead of num_devices. This would fix the problem partly. Partly because ealier num_devices included the replacing device but now total_device does not include the replacing device. Getting a count which includes a transient device is rather too in efficient/wrong indeed, because there can be a race condition where in the time between ioctl BTRFS_IOC_FS_INFO to BTRFS_IOC_DEV_INFO the replace device operation might have been completed. So to fix this problem its better that user land btrfs-progs probes replacing device (at devid 0) separately. v2: Agree with Wang's comment. Its better to show seed disks under the sprout fs, so that user can establish mapping of seed to sprout devices. So here I am making BTRFS_IOC_FS_INFO to return the total_devices which would count the seed devices (but not the replacing device). A btrfs-progs patch will separately probe for devid 0 (which is a replacing transient device when present). v1: This patch will make BTRFS_IOC_DEV_INFO ioctl to provide disks only of the FSID being probed (seed disks are under different FSID). which means when seed is still not deleted from the sprout the btrfs filesystem show command will show disks them under their respective FSIDs Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: a new kind of fix. which would allow seed to be shown in the btrfs fi show cli output. fs/btrfs/ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)