| Message ID | 20201118084800.2339180-20-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/20] blk-cgroup: fix a hd_struct leak in blkcg_fill_root_iostats | expand |
On 11/18/20 4:47 PM, Christoph Hellwig wrote: > Don't bother to call lookup_bdev for just a slightly different error > message without any functional change. > > Signed-off-by: Christoph Hellwig <hch@lst.de>ist Hi Christoph, NACK. This removing error message is frequently triggered and observed, and distinct a busy device and an already registered device is important (the first one is critical error and second one is not). Remove such error message will be a functional regression. Coly Li > --- > drivers/md/bcache/super.c | 44 +-------------------------------------- > 1 file changed, 1 insertion(+), 43 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index e5db2cdd114112..5c531ed7785280 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -2380,40 +2380,6 @@ kobj_attribute_write(register, register_bcache); > kobj_attribute_write(register_quiet, register_bcache); > kobj_attribute_write(pendings_cleanup, bch_pending_bdevs_cleanup); > > -static bool bch_is_open_backing(struct block_device *bdev) > -{ > - struct cache_set *c, *tc; > - struct cached_dev *dc, *t; > - > - list_for_each_entry_safe(c, tc, &bch_cache_sets, list) > - list_for_each_entry_safe(dc, t, &c->cached_devs, list) > - if (dc->bdev == bdev) > - return true; > - list_for_each_entry_safe(dc, t, &uncached_devices, list) > - if (dc->bdev == bdev) > - return true; > - return false; > -} > - > -static bool bch_is_open_cache(struct block_device *bdev) > -{ > - struct cache_set *c, *tc; > - > - list_for_each_entry_safe(c, tc, &bch_cache_sets, list) { > - struct cache *ca = c->cache; > - > - if (ca->bdev == bdev) > - return true; > - } > - > - return false; > -} > - > -static bool bch_is_open(struct block_device *bdev) > -{ > - return bch_is_open_cache(bdev) || bch_is_open_backing(bdev); > -} > - > struct async_reg_args { > struct delayed_work reg_work; > char *path; > @@ -2535,15 +2501,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, > sb); > if (IS_ERR(bdev)) { > if (bdev == ERR_PTR(-EBUSY)) { > - bdev = lookup_bdev(strim(path)); > - mutex_lock(&bch_register_lock); > - if (!IS_ERR(bdev) && bch_is_open(bdev)) > - err = "device already registered"; > - else > - err = "device busy"; > - mutex_unlock(&bch_register_lock); > - if (!IS_ERR(bdev)) > - bdput(bdev); > + err = "device busy"; > if (attr == &ksysfs_register_quiet) > goto done; > } >
On Wed, Nov 18, 2020 at 04:54:51PM +0800, Coly Li wrote: > On 11/18/20 4:47 PM, Christoph Hellwig wrote: > > Don't bother to call lookup_bdev for just a slightly different error > > message without any functional change. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>ist > > Hi Christoph, > > NACK. This removing error message is frequently triggered and observed, > and distinct a busy device and an already registered device is important > (the first one is critical error and second one is not). > > Remove such error message will be a functional regression. What normal operation causes this error message to be emitted? And what can a user do with it? thanks, greg k-h
On 11/18/20 5:10 PM, Greg KH wrote: > On Wed, Nov 18, 2020 at 04:54:51PM +0800, Coly Li wrote: >> On 11/18/20 4:47 PM, Christoph Hellwig wrote: >>> Don't bother to call lookup_bdev for just a slightly different error >>> message without any functional change. >>> >>> Signed-off-by: Christoph Hellwig <hch@lst.de>ist >> >> Hi Christoph, >> >> NACK. This removing error message is frequently triggered and observed, >> and distinct a busy device and an already registered device is important >> (the first one is critical error and second one is not). >> >> Remove such error message will be a functional regression. > > What normal operation causes this error message to be emitted? And what > can a user do with it? When there was bug and the caching or backing device was not unregistered successfully, people could see "device busy"; and if it was because the device registered again, it could be "already registered". Without the different message, people may think the device is always busy but indeed it isn't. he motivation of the patch is OK to me, but we need to make the logical consistent, otherwise we will have similar bug report for bogus warning dmesg from bcache users in soon future. Coly Li
On Wed, Nov 18, 2020 at 04:54:51PM +0800, Coly Li wrote: > On 11/18/20 4:47 PM, Christoph Hellwig wrote: > > Don't bother to call lookup_bdev for just a slightly different error > > message without any functional change. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>ist > > Hi Christoph, > > NACK. This removing error message is frequently triggered and observed, > and distinct a busy device and an already registered device is important > (the first one is critical error and second one is not). > > Remove such error message will be a functional regression. I can probably keep it, the amount of code to prettiefy an error message seems excessive, though.
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index e5db2cdd114112..5c531ed7785280 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -2380,40 +2380,6 @@ kobj_attribute_write(register, register_bcache); kobj_attribute_write(register_quiet, register_bcache); kobj_attribute_write(pendings_cleanup, bch_pending_bdevs_cleanup); -static bool bch_is_open_backing(struct block_device *bdev) -{ - struct cache_set *c, *tc; - struct cached_dev *dc, *t; - - list_for_each_entry_safe(c, tc, &bch_cache_sets, list) - list_for_each_entry_safe(dc, t, &c->cached_devs, list) - if (dc->bdev == bdev) - return true; - list_for_each_entry_safe(dc, t, &uncached_devices, list) - if (dc->bdev == bdev) - return true; - return false; -} - -static bool bch_is_open_cache(struct block_device *bdev) -{ - struct cache_set *c, *tc; - - list_for_each_entry_safe(c, tc, &bch_cache_sets, list) { - struct cache *ca = c->cache; - - if (ca->bdev == bdev) - return true; - } - - return false; -} - -static bool bch_is_open(struct block_device *bdev) -{ - return bch_is_open_cache(bdev) || bch_is_open_backing(bdev); -} - struct async_reg_args { struct delayed_work reg_work; char *path; @@ -2535,15 +2501,7 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, sb); if (IS_ERR(bdev)) { if (bdev == ERR_PTR(-EBUSY)) { - bdev = lookup_bdev(strim(path)); - mutex_lock(&bch_register_lock); - if (!IS_ERR(bdev) && bch_is_open(bdev)) - err = "device already registered"; - else - err = "device busy"; - mutex_unlock(&bch_register_lock); - if (!IS_ERR(bdev)) - bdput(bdev); + err = "device busy"; if (attr == &ksysfs_register_quiet) goto done; }
Don't bother to call lookup_bdev for just a slightly different error message without any functional change. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/bcache/super.c | 44 +-------------------------------------- 1 file changed, 1 insertion(+), 43 deletions(-)