| Message ID | 1498855388-16990-1-git-send-email-bcache@lists.ewheeler.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017/7/1 上午4:42, bcache@lists.ewheeler.net wrote: > From: Jan Kara <jack@suse.cz> > > If blkdev_get_by_path() in register_bcache() fails, we try to lookup the > block device using lookup_bdev() to detect which situation we are in to > properly report error. However we never drop the reference returned to > us from lookup_bdev(). Fix that. > > Signed-off-by: Jan Kara <jack@suse.cz> > Cc: stable@vger.kernel.org Acked-by: Coly Li <colyli@suse.de> Thanks. > --- > drivers/md/bcache/super.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index 8352fad..9a2c190 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, > else > err = "device busy"; > mutex_unlock(&bch_register_lock); > + if (!IS_ERR(bdev)) > + bdput(bdev); > if (attr == &ksysfs_register_quiet) > goto out; > } >
On Fri, Jun 30, 2017 at 01:42:50PM -0700, bcache@lists.ewheeler.net wrote: > From: Jan Kara <jack@suse.cz> > > If blkdev_get_by_path() in register_bcache() fails, we try to lookup the > block device using lookup_bdev() to detect which situation we are in to > properly report error. However we never drop the reference returned to > us from lookup_bdev(). Fix that. This look ok, but I think that whole chunk of code should just go away - adding a lookup_bdev and resulting mess just for a slightly different error message is just insane.
> +#define BCACHE_TO_IDA_MINORS(first_minor) ((first_minor) >> BCACHE_MINORS_BITS) > +#define IDA_TO_BCACHE_MINORS(minor) ((minor) << BCACHE_MINORS_BITS) Please use inline functions and lower case for these.
On 2017/7/6 上午2:24, Christoph Hellwig wrote: > On Fri, Jun 30, 2017 at 01:42:50PM -0700, bcache@lists.ewheeler.net wrote: >> From: Jan Kara <jack@suse.cz> >> >> If blkdev_get_by_path() in register_bcache() fails, we try to lookup the >> block device using lookup_bdev() to detect which situation we are in to >> properly report error. However we never drop the reference returned to >> us from lookup_bdev(). Fix that. > > This look ok, but I think that whole chunk of code should just go > away - adding a lookup_bdev and resulting mess just for a slightly > different error message is just insane. Hi Christoph, When you mentioned "whole chunk of code", do you mean the following block of code ? 1960 if (IS_ERR(bdev)) { ========= start of whole chunk of code ============ 1961 if (bdev == ERR_PTR(-EBUSY)) { 1962 bdev = lookup_bdev(strim(path)); 1963 mutex_lock(&bch_register_lock); 1964 if (!IS_ERR(bdev) && bch_is_open(bdev)) 1965 err = "device already registered"; 1966 else 1967 err = "device busy"; 1968 mutex_unlock(&bch_register_lock); 1969 if (!IS_ERR(bdev)) 1970 bdput(bdev); 1971 if (attr == &ksysfs_register_quiet) 1972 goto out; 1973 } ========= end of whole chunk of code ============ 1974 goto err; 1975 } I don't mind to remove it, just double check I don't misunderstand what you meant. Thanks.
On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote: > > When you mentioned "whole chunk of code", do you mean the following > block of code ? > > > 1960 if (IS_ERR(bdev)) { > ========= start of whole chunk of code ============ > 1961 if (bdev == ERR_PTR(-EBUSY)) { > 1962 bdev = lookup_bdev(strim(path)); > 1963 mutex_lock(&bch_register_lock); > 1964 if (!IS_ERR(bdev) && bch_is_open(bdev)) > 1965 err = "device already registered"; > 1966 else > 1967 err = "device busy"; > 1968 mutex_unlock(&bch_register_lock); > 1969 if (!IS_ERR(bdev)) > 1970 bdput(bdev); > 1971 if (attr == &ksysfs_register_quiet) > 1972 goto out; > 1973 } > ========= end of whole chunk of code ============ > 1974 goto err; > 1975 } > > I don't mind to remove it, just double check I don't misunderstand what > you meant. Yes, that's the problematic block.
On 2017/9/5 下午2:43, Christoph Hellwig wrote: > On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote: >> >> When you mentioned "whole chunk of code", do you mean the following >> block of code ? >> >> >> 1960 if (IS_ERR(bdev)) { >> ========= start of whole chunk of code ============ >> 1961 if (bdev == ERR_PTR(-EBUSY)) { >> 1962 bdev = lookup_bdev(strim(path)); >> 1963 mutex_lock(&bch_register_lock); >> 1964 if (!IS_ERR(bdev) && bch_is_open(bdev)) >> 1965 err = "device already registered"; >> 1966 else >> 1967 err = "device busy"; >> 1968 mutex_unlock(&bch_register_lock); >> 1969 if (!IS_ERR(bdev)) >> 1970 bdput(bdev); >> 1971 if (attr == &ksysfs_register_quiet) >> 1972 goto out; >> 1973 } >> ========= end of whole chunk of code ============ >> 1974 goto err; >> 1975 } >> >> I don't mind to remove it, just double check I don't misunderstand what >> you meant. > > Yes, that's the problematic block. > Understand, I will send out a patch, hopefully it can catch up 4.14 merge window. Thanks for the hint.
On 2017/9/5 下午2:43, Christoph Hellwig wrote: > On Tue, Sep 05, 2017 at 01:30:04AM +0800, Coly Li wrote: >> >> When you mentioned "whole chunk of code", do you mean the following >> block of code ? >> >> >> 1960 if (IS_ERR(bdev)) { >> ========= start of whole chunk of code ============ >> 1961 if (bdev == ERR_PTR(-EBUSY)) { >> 1962 bdev = lookup_bdev(strim(path)); >> 1963 mutex_lock(&bch_register_lock); >> 1964 if (!IS_ERR(bdev) && bch_is_open(bdev)) >> 1965 err = "device already registered"; >> 1966 else >> 1967 err = "device busy"; >> 1968 mutex_unlock(&bch_register_lock); >> 1969 if (!IS_ERR(bdev)) >> 1970 bdput(bdev); >> 1971 if (attr == &ksysfs_register_quiet) >> 1972 goto out; >> 1973 } >> ========= end of whole chunk of code ============ >> 1974 goto err; >> 1975 } >> >> I don't mind to remove it, just double check I don't misunderstand what >> you meant. > > Yes, that's the problematic block. > Hi Christoph, I tested the code, and my patch which removes the above code. The result is, I feel the above chunk of code is useful. When I tried to register a device and it was already registered. Without the above code, I only saw "failed to open device", didn't realize it was because this device is registered already. After adding the above code back, I knew where the problem was. The above chunk of code improves user experience and provides more detailed diagnose information, it is useful. Then I suggest to keep the code here and pick up Jan's patch. Thanks.
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index 8352fad..9a2c190 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -1964,6 +1964,8 @@ static ssize_t register_bcache(struct kobject *k, struct kobj_attribute *attr, else err = "device busy"; mutex_unlock(&bch_register_lock); + if (!IS_ERR(bdev)) + bdput(bdev); if (attr == &ksysfs_register_quiet) goto out; }