| Message ID | 20230629165206.383-1-jack@suse.cz |
|---|---|
| Headers | show |
| Series | block: Make blkdev_get_by_*() return handle | expand |
On Tue 04-07-23 13:43:51, Matthew Wilcox wrote: > On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > > +struct bdev_handle *blkdev_get_handle_by_dev(dev_t dev, blk_mode_t mode, > > + void *holder, const struct blk_holder_ops *hops) > > +{ > > + struct bdev_handle *handle = kmalloc(sizeof(struct bdev_handle), > > + GFP_KERNEL); > > + struct block_device *bdev; > > + > > + if (!handle) > > + return ERR_PTR(-ENOMEM); > > + bdev = blkdev_get_by_dev(dev, mode, holder, hops); > > + if (IS_ERR(bdev)) > > + return ERR_CAST(bdev); > > Would we be better off with a handle->error (and a NULL return from this > function means "we couldn't allocate a handle")? I have no objection > to what you've done here, just wondering if it might end up nicer for > the users. Hum, I've checked a couple of users and it seems it would be more complicated for the users to handle this convention than the one I've chosen. And that one is also pretty standard so I think by the principle of least surprise it is also better. Honza >
On Tue 04-07-23 10:28:36, Keith Busch wrote: > On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > > +struct bdev_handle *blkdev_get_handle_by_dev(dev_t dev, blk_mode_t mode, > > + void *holder, const struct blk_holder_ops *hops) > > +{ > > + struct bdev_handle *handle = kmalloc(sizeof(struct bdev_handle), > > + GFP_KERNEL); > > I believe 'sizeof(*handle)' is the preferred style. OK. > > + struct block_device *bdev; > > + > > + if (!handle) > > + return ERR_PTR(-ENOMEM); > > + bdev = blkdev_get_by_dev(dev, mode, holder, hops); > > + if (IS_ERR(bdev)) > > + return ERR_CAST(bdev); > > Need a 'kfree(handle)' before the error return. Or would it be simpler > to get the bdev first so you can check the mode settings against a > read-only bdev prior to the kmalloc? Yeah. Good point with kfree(). I'm not sure calling blkdev_get_by_dev() first will be "simpler" - then we need blkdev_put() in case of kmalloc() failure. Thanks for review! Honza
On 7/4/23 09:14, Matthew Wilcox wrote: > On Tue, Jul 04, 2023 at 07:06:26AM -0700, Bart Van Assche wrote: >> On 7/4/23 05:21, Jan Kara wrote: >>> +struct bdev_handle { >>> + struct block_device *bdev; >>> + void *holder; >>> +}; >> >> Please explain in the patch description why a holder pointer is introduced >> in struct bdev_handle and how it relates to the bd_holder pointer in struct >> block_device. Is one of the purposes of this patch series perhaps to add >> support for multiple holders per block device? > > That is all in patch 0/32. Why repeat it? This cover letter: https://lore.kernel.org/linux-block/20230629165206.383-1-jack@suse.cz/T/#t? The word "holder" doesn't even occur in that cover letter so how could the answer to my question be present in the cover letter? Bart.
On Tue, Jul 04, 2023 at 02:21:27PM +0200, Jan Kara wrote: > Hello, > > this patch series implements the idea of blkdev_get_by_*() calls returning > bdev_handle which is then passed to blkdev_put() [1]. This makes the get > and put calls for bdevs more obviously matching and allows us to propagate > context from get to put without having to modify all the users (again!). > In particular I need to propagate used open flags to blkdev_put() to be able > count writeable opens and add support for blocking writes to mounted block > devices. I'll send that series separately. > > The series is based on Linus' tree as of yesterday + two bcache fixes which are > in the block tree. Patches have passed some basic testing, I plan to test more > users once we agree this is the right way to go. Can you post a link to a git branch for this and the follow up series? Especially with a fairly unstable base it's kinda hard to look at the result otherwise.