| Message ID | 20171020140715.3110-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 20, 2017 at 10:07:15PM +0800, Anand Jain wrote: > We aren't setting the FMODE_WRITE when initializing btrfs_device > structure and when calling blkdev_put, however we are setting it > only when calling blkdev_get_by_path(). But this still does not say why this is a problem worth fixing. Nikolay asked for it, and I would do the same, but why do we even have to ask for that? https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes -- 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 10/30/2017 10:39 PM, David Sterba wrote: > On Fri, Oct 20, 2017 at 10:07:15PM +0800, Anand Jain wrote: >> We aren't setting the FMODE_WRITE when initializing btrfs_device >> structure and when calling blkdev_put, however we are setting it >> only when calling blkdev_get_by_path(). > > But this still does not say why this is a problem worth fixing. Nikolay > asked for it, and I would do the same, but why do we even have to ask > for that? Here its just a cleanup of miss match of open mode and close modes. And there isn't any problem that I noticed. Thanks, Anand > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes > -- > 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 31.10.2017 04:11, Anand Jain wrote: > > > On 10/30/2017 10:39 PM, David Sterba wrote: >> On Fri, Oct 20, 2017 at 10:07:15PM +0800, Anand Jain wrote: >>> We aren't setting the FMODE_WRITE when initializing btrfs_device >>> structure and when calling blkdev_put, however we are setting it >>> only when calling blkdev_get_by_path(). >> >> But this still does not say why this is a problem worth fixing. Nikolay >> asked for it, and I would do the same, but why do we even have to ask >> for that? > > Here its just a cleanup of miss match of open mode and close modes.> And there isn't any problem that I noticed. Even if that's the case, please state that explicitly in your changelog and also put "No functional changes" if you expect it to not introduce any change in behavior. > > Thanks, Anand > > >> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes >> >> -- >> 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
On 10/31/2017 10:21 PM, Nikolay Borisov wrote: > > > On 31.10.2017 04:11, Anand Jain wrote: >> >> >> On 10/30/2017 10:39 PM, David Sterba wrote: >>> On Fri, Oct 20, 2017 at 10:07:15PM +0800, Anand Jain wrote: >>>> We aren't setting the FMODE_WRITE when initializing btrfs_device >>>> structure and when calling blkdev_put, however we are setting it >>>> only when calling blkdev_get_by_path(). >>> >>> But this still does not say why this is a problem worth fixing. Nikolay >>> asked for it, and I would do the same, but why do we even have to ask >>> for that? >> >> Here its just a cleanup of miss match of open mode and close modes.> And there isn't any problem that I noticed. > > Even if that's the case, please state that explicitly in your changelog > and also put "No functional changes" if you expect it to not introduce > any change in behavior. > Ah. Will update change log. Thanks, Anand >> >> Thanks, Anand >> >> >>> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes >>> >>> -- >>> 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
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index b39737568c22..765c2bd2d8d9 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -2323,12 +2323,12 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path u64 tmp; int seeding_dev = 0; int ret = 0; + fmode_t flag = FMODE_WRITE | FMODE_EXCL; if (sb_rdonly(sb) && !fs_info->fs_devices->seeding) return -EROFS; - bdev = blkdev_get_by_path(device_path, FMODE_WRITE | FMODE_EXCL, - fs_info->bdev_holder); + bdev = blkdev_get_by_path(device_path, flag, fs_info->bdev_holder); if (IS_ERR(bdev)) return PTR_ERR(bdev); @@ -2392,7 +2392,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path device->bdev = bdev; device->in_fs_metadata = 1; device->is_tgtdev_for_dev_replace = 0; - device->mode = FMODE_EXCL; + device->mode = flag; device->dev_stats_valid = 1; set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); @@ -2506,7 +2506,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path btrfs_sysfs_rm_device_link(fs_info->fs_devices, device); kfree(device); error: - blkdev_put(bdev, FMODE_EXCL); + blkdev_put(bdev, flag); if (seeding_dev) { mutex_unlock(&uuid_mutex); up_write(&sb->s_umount); @@ -2526,6 +2526,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, struct rcu_string *name; u64 devid = BTRFS_DEV_REPLACE_DEVID; int ret = 0; + fmode_t flags = FMODE_WRITE | FMODE_EXCL; *device_out = NULL; if (fs_info->fs_devices->seeding) { @@ -2533,8 +2534,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, return -EINVAL; } - bdev = blkdev_get_by_path(device_path, FMODE_WRITE | FMODE_EXCL, - fs_info->bdev_holder); + bdev = blkdev_get_by_path(device_path, flags, fs_info->bdev_holder); if (IS_ERR(bdev)) { btrfs_err(fs_info, "target device %s is invalid!", device_path); return PTR_ERR(bdev); @@ -2595,7 +2595,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, device->bdev = bdev; device->in_fs_metadata = 1; device->is_tgtdev_for_dev_replace = 1; - device->mode = FMODE_EXCL; + device->mode = flags; device->dev_stats_valid = 1; set_blocksize(device->bdev, BTRFS_BDEV_BLOCKSIZE); device->fs_devices = fs_info->fs_devices; @@ -2608,7 +2608,7 @@ int btrfs_init_dev_replace_tgtdev(struct btrfs_fs_info *fs_info, return ret; error: - blkdev_put(bdev, FMODE_EXCL); + blkdev_put(bdev, flags); return ret; }
We aren't setting the FMODE_WRITE when initializing btrfs_device structure and when calling blkdev_put, however we are setting it only when calling blkdev_get_by_path(). Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: Reword commit log. fs/btrfs/volumes.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)