From: Eric Wheeler <bcache@lists.ewheeler.net>
To: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org, linux-block@vger.kernel.org,
Christoph Hellwig <hch@infradead.org>,
linux-bcache@vger.kernel.org,
Kent Overstreet <kent.overstreet@gmail.com>,
Coly Li <colyli@suse.de>
Subject: Re: [PATCH 09/29] bcache: Convert to bdev_open_by_path()
Date: Mon, 21 Aug 2023 11:54:04 -0700 (PDT) [thread overview]
Message-ID: <4c14b62-22a-e9b4-ab2d-3272d0c0495e@ewheeler.net> (raw)
In-Reply-To: <20230821175053.osjvbwnubr2k6q5q@quack3>
On Mon, 21 Aug 2023, Jan Kara wrote:
> On Sun 20-08-23 18:06:01, Eric Wheeler wrote:
> > On Fri, 11 Aug 2023, Jan Kara wrote:
> > > Convert bcache to use bdev_open_by_path() and pass the handle around.
> > >
> > > CC: linux-bcache@vger.kernel.org
> > > CC: Coly Li <colyli@suse.de
> > > CC: Kent Overstreet <kent.overstreet@gmail.com>
> > > Acked-by: Coly Li <colyli@suse.de>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > > drivers/md/bcache/bcache.h | 2 +
> > > drivers/md/bcache/super.c | 78 ++++++++++++++++++++------------------
> > > 2 files changed, 43 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> > > index 5a79bb3c272f..2aa3f2c1f719 100644
> > > --- a/drivers/md/bcache/bcache.h
> > > +++ b/drivers/md/bcache/bcache.h
> > > @@ -299,6 +299,7 @@ struct cached_dev {
> > > struct list_head list;
> > > struct bcache_device disk;
> > > struct block_device *bdev;
> > > + struct bdev_handle *bdev_handle;
> >
> > It looks like you've handled most if not all of the `block_device *bdev`
> > refactor. Can we drop `block_device *bdev` and fixup any remaining
> > references? More below.
>
> Well, we could but it's a lot of churn - like 53 dereferences in bcache.
> So if bcache maintainer wants to go this way, sure we can do it. But
> preferably as a separate cleanup patch on top of this series because the
> series generates enough conflicts as is and this will make it considerably
> worse.
A separate cleanup patch seems reasonable, I'll defer to Coly on this one
since he's the maintainer. I just wanted to point out the possible issue.
Thanks for your work on this.
--
Eric Wheeler
>
> > > @@ -421,6 +422,7 @@ struct cache {
> > >
> > > struct kobject kobj;
> > > struct block_device *bdev;
> > > + struct bdev_handle *bdev_handle;
> >
> > ditto.
> >
> > >
> > > struct task_struct *alloc_thread;
> > >
> > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > > index 0ae2b3676293..c11ac86be72b 100644
> > > --- a/drivers/md/bcache/super.c
> > > +++ b/drivers/md/bcache/super.c
> > > @@ -1368,8 +1368,8 @@ static void cached_dev_free(struct closure *cl)
> > > if (dc->sb_disk)
> > > put_page(virt_to_page(dc->sb_disk));
> > >
> > > - if (!IS_ERR_OR_NULL(dc->bdev))
> > > - blkdev_put(dc->bdev, dc);
> > > + if (dc->bdev_handle)
> > > + bdev_release(dc->bdev_handle);
> >
> > bdev_release does not reset dc->bdev, which could leave a hanging
> > reference.
>
> So after this, dc->bdev may reference freed block device that is true. But
> the original code did not cleanup dc->bdev either so things just stay as
> they were.
>
> > > @@ -1444,7 +1444,7 @@ static int cached_dev_init(struct cached_dev *dc, unsigned int block_size)
> > > /* Cached device - bcache superblock */
> > >
> > > static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> > > - struct block_device *bdev,
> > > + struct bdev_handle *bdev_handle,
> > > struct cached_dev *dc)
> > > {
> > > const char *err = "cannot allocate memory";
> > > @@ -1452,14 +1452,15 @@ static int register_bdev(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> > > int ret = -ENOMEM;
> > >
> > > memcpy(&dc->sb, sb, sizeof(struct cache_sb));
> > > - dc->bdev = bdev;
> > > + dc->bdev_handle = bdev_handle;
> > > + dc->bdev = bdev_handle->bdev;
> >
> > If I understand correctly, this patch duplicates the dc->bdev reference to
> > exist as dc->bdev_handle->bdev _and_ dc->bdev. (Same for changes related
> > to `struct cache`.)
>
> Well, dc->bdev isn't a reference anymore, just a shortcut so that people
> don't have to write the long dc->bdev_handle->bdev (plus it limits the
> churn this series generates as I've mentioned above). I can see why some
> people needn't like this duplication so sure we can clean it up if that's
> the concensus of bcache developers.
>
> > This would mean future developers have to understand they are the same
> > thing, and someone may not manage it correctly.
> >
> > If block core is moving to `struct bdev_handle`, then can we drop
> > `dc->bdev` and replace all occurances of `dc->bdev` with
> > `bdev_handle->bdev`? Or make an accessor macro/function like
> > bdev_handle_get_bdev(dc->bdev_handle)?
>
> Accessor is making things even longer and I don't see the benefit. So I'd
> just go with dc->bdev_handle->bdev.
>
> > Unless I misunderstand something here, I would NACK this as written
> > because it increases the liklihood of future developer error.
> >
> > I've added a few other comments below, but my comments are not exhaustive:
> >
> > > dc->sb_disk = sb_disk;
> > >
> > > if (cached_dev_init(dc, sb->block_size << 9))
> > > goto err;
> > >
> > > err = "error creating kobject";
> > > - if (kobject_add(&dc->disk.kobj, bdev_kobj(bdev), "bcache"))
> > > + if (kobject_add(&dc->disk.kobj, bdev_kobj(dc->bdev), "bcache"))
> > > goto err;
> > > if (bch_cache_accounting_add_kobjs(&dc->accounting, &dc->disk.kobj))
> > > goto err;
> > > @@ -2216,8 +2217,8 @@ void bch_cache_release(struct kobject *kobj)
> > > if (ca->sb_disk)
> > > put_page(virt_to_page(ca->sb_disk));
> > >
> > > - if (!IS_ERR_OR_NULL(ca->bdev))
> > > - blkdev_put(ca->bdev, ca);
> > > + if (ca->bdev_handle)
> > > + bdev_release(ca->bdev_handle);
> > >
> >
> > ca->bdev is not cleaned up
>
> Well, same comment as with dc->bdev - the old code didn't cleanup the
> pointer either. Furthermore the structure is kfree()d in the line below so
> there is really no point in zeroing the pointer.
>
> > > kfree(ca);
> > > module_put(THIS_MODULE);
> > > @@ -2337,16 +2338,18 @@ static int cache_alloc(struct cache *ca)
> > > }
> > >
> > > static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> > > - struct block_device *bdev, struct cache *ca)
> > > + struct bdev_handle *bdev_handle,
> > > + struct cache *ca)
> > > {
> > > const char *err = NULL; /* must be set for any error case */
> > > int ret = 0;
> > >
> > > memcpy(&ca->sb, sb, sizeof(struct cache_sb));
> > > - ca->bdev = bdev;
> > > + ca->bdev_handle = bdev_handle;
> > > + ca->bdev = bdev_handle->bdev;
> > > ca->sb_disk = sb_disk;
> > >
> > > - if (bdev_max_discard_sectors((bdev)))
> > > + if (bdev_max_discard_sectors((bdev_handle->bdev)))
> > > ca->discard = CACHE_DISCARD(&ca->sb);
> > >
> > > ret = cache_alloc(ca);
> > > @@ -2354,10 +2357,10 @@ static int register_cache(struct cache_sb *sb, struct cache_sb_disk *sb_disk,
> > > /*
> > > * If we failed here, it means ca->kobj is not initialized yet,
> > > * kobject_put() won't be called and there is no chance to
> > > - * call blkdev_put() to bdev in bch_cache_release(). So we
> > > - * explicitly call blkdev_put() here.
> > > + * call bdev_release() to bdev in bch_cache_release(). So
> > > + * we explicitly call bdev_release() here.
> > > */
> > > - blkdev_put(bdev, ca);
> > > + bdev_release(bdev_handle);
> >
> > ca->bdev is not cleaned up
>
> So ca->bdev doesn't really need to be cleaned up here and the original code
> wasn't cleaning it up either. So I don't see a problem here either... But
> maybe I miss something.
>
> Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>
next prev parent reply other threads:[~2023-08-21 18:56 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-11 11:04 [PATCH v2 0/29] block: Make blkdev_get_by_*() return handle Jan Kara
2023-08-11 11:04 ` [PATCH 09/29] bcache: Convert to bdev_open_by_path() Jan Kara
2023-08-21 1:06 ` Eric Wheeler
2023-08-21 17:50 ` Jan Kara
2023-08-21 18:54 ` Eric Wheeler [this message]
2023-08-23 10:10 ` Coly Li
2023-08-11 12:27 ` [PATCH v2 0/29] block: Make blkdev_get_by_*() return handle Christoph Hellwig
2023-08-25 1:58 ` Al Viro
2023-08-25 13:47 ` Jan Kara
2023-08-26 2:28 ` Al Viro
2023-08-28 14:27 ` Christoph Hellwig
2023-08-28 13:20 ` Christian Brauner
2023-08-28 14:22 ` Christoph Hellwig
2023-08-23 10:48 [PATCH v3 " Jan Kara
2023-08-23 10:48 ` [PATCH 09/29] bcache: Convert to bdev_open_by_path() Jan Kara
2023-08-25 12:06 ` Christian Brauner
2023-09-27 9:34 ` Jan Kara
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4c14b62-22a-e9b4-ab2d-3272d0c0495e@ewheeler.net \
--to=bcache@lists.ewheeler.net \
--cc=colyli@suse.de \
--cc=hch@infradead.org \
--cc=jack@suse.cz \
--cc=kent.overstreet@gmail.com \
--cc=linux-bcache@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).