| Message ID | 20230704122224.16257-1-jack@suse.cz (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | block: Make blkdev_get_by_*() return handle | expand |
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.
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 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? Thanks, Bart.
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?
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. > + 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? > + handle->bdev = bdev; > + handle->holder = holder; > + return handle; > +} > +EXPORT_SYMBOL(blkdev_get_handle_by_dev); > + > /** > * blkdev_get_by_path - open a block device by name > * @path: path to the block device to open > @@ -884,6 +902,28 @@ struct block_device *blkdev_get_by_path(const char *path, blk_mode_t mode, > } > EXPORT_SYMBOL(blkdev_get_by_path); > > +struct bdev_handle *blkdev_get_handle_by_path(const char *path, blk_mode_t mode, > + void *holder, const struct blk_holder_ops *hops) > +{ > + struct bdev_handle *handle; > + dev_t dev; > + int error; > + > + error = lookup_bdev(path, &dev); > + if (error) > + return ERR_PTR(error); > + > + handle = blkdev_get_handle_by_dev(dev, mode, holder, hops); > + if (!IS_ERR(handle) && (mode & BLK_OPEN_WRITE) && > + bdev_read_only(handle->bdev)) { > + blkdev_handle_put(handle); > + return ERR_PTR(-EACCES); > + } > + > + return handle; > +} > +EXPORT_SYMBOL(blkdev_get_handle_by_path);
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 04-07-23 07:06:26, 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? No. The reason for adding holder to struct bdev_handle is that it is an argument blkdev_put() needs. Currently, every user of blkdev_put() has to remember what it has passed as 'holder' to blkdev_get_by_*() call and pass that to blkdev_put(). With struct bdev_handle this will happen automatically. This is already explained in the changelog of this patch: "Create struct bdev_handle that contains all parameters that need to be passed to blkdev_put()..." If it was only about holder, the intrusive patches would not be warranted but as the description also says: "This will eventually allow us to pass one more argument to blkdev_put() without too much hassle." Because we will additionaly need to propagate the 'mode' argument used at open to blkdev_put(). Honza
On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > Create struct bdev_handle that contains all parameters that need to be > passed to blkdev_put() and provide blkdev_get_handle_* functions that > return this structure instead of plain bdev pointer. This will > eventually allow us to pass one more argument to blkdev_put() without > too much hassle. Can we use the opportunity to come up with better names? blkdev_get_* was always a rather horrible naming convention for something that ends up calling into ->open. What about: struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, const struct blk_holder_ops *hops); struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, void *holder, const struct blk_holder_ops *hops); void bdev_release(struct bdev_handle *handle); ?
On Thu 06-07-23 08:38:40, Christoph Hellwig wrote: > On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > > Create struct bdev_handle that contains all parameters that need to be > > passed to blkdev_put() and provide blkdev_get_handle_* functions that > > return this structure instead of plain bdev pointer. This will > > eventually allow us to pass one more argument to blkdev_put() without > > too much hassle. > > Can we use the opportunity to come up with better names? blkdev_get_* > was always a rather horrible naming convention for something that > ends up calling into ->open. > > What about: > > struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, > const struct blk_holder_ops *hops); > struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, > void *holder, const struct blk_holder_ops *hops); > void bdev_release(struct bdev_handle *handle); I'd maybe use bdev_close() instead of bdev_release() but otherwise I like the new naming. Honza
On Thu, Jul 06, 2023 at 06:14:33PM +0200, Jan Kara wrote: > > struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, > > void *holder, const struct blk_holder_ops *hops); > > void bdev_release(struct bdev_handle *handle); > > I'd maybe use bdev_close() instead of bdev_release() but otherwise I like > the new naming. We're using release everywhese else, but if Jens is fine with that I can live with close.
On Fri 07-07-23 04:28:41, Christoph Hellwig wrote: > On Thu, Jul 06, 2023 at 06:14:33PM +0200, Jan Kara wrote: > > > struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, > > > void *holder, const struct blk_holder_ops *hops); > > > void bdev_release(struct bdev_handle *handle); > > > > I'd maybe use bdev_close() instead of bdev_release() but otherwise I like > > the new naming. > > We're using release everywhese else, but if Jens is fine with that I > can live with close. Dunno, to me words pair like open-close, get-put, acquire-release. Furthermore e.g. ->release() (and thus blkdev_release()) is called only when the last file reference is dropped, not when each reference is dropped, so that's why bdev_release() seems a bit confusing to me. Honza
On Thu, Jul 6, 2023 at 5:38 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > > Create struct bdev_handle that contains all parameters that need to be > > passed to blkdev_put() and provide blkdev_get_handle_* functions that > > return this structure instead of plain bdev pointer. This will > > eventually allow us to pass one more argument to blkdev_put() without > > too much hassle. > > Can we use the opportunity to come up with better names? blkdev_get_* > was always a rather horrible naming convention for something that > ends up calling into ->open. > > What about: > > struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, > const struct blk_holder_ops *hops); > struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, > void *holder, const struct blk_holder_ops *hops); > void bdev_release(struct bdev_handle *handle); +1 to this. Also, if we are removing "handle" from the function, should the name of the structure it returns also change? Would something like bdev_ctx be better? (Apologies for the previous non-plaintext email) > > ?
On Wed 12-07-23 18:06:35, Haris Iqbal wrote: > On Thu, Jul 6, 2023 at 5:38 PM Christoph Hellwig <hch@infradead.org> wrote: > > > > On Tue, Jul 04, 2023 at 02:21:28PM +0200, Jan Kara wrote: > > > Create struct bdev_handle that contains all parameters that need to be > > > passed to blkdev_put() and provide blkdev_get_handle_* functions that > > > return this structure instead of plain bdev pointer. This will > > > eventually allow us to pass one more argument to blkdev_put() without > > > too much hassle. > > > > Can we use the opportunity to come up with better names? blkdev_get_* > > was always a rather horrible naming convention for something that > > ends up calling into ->open. > > > > What about: > > > > struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder, > > const struct blk_holder_ops *hops); > > struct bdev_handle *bdev_open_by_path(dev_t dev, blk_mode_t mode, > > void *holder, const struct blk_holder_ops *hops); > > void bdev_release(struct bdev_handle *handle); > > +1 to this. > Also, if we are removing "handle" from the function, should the name > of the structure it returns also change? Would something like bdev_ctx > be better? I think the bdev_handle name is fine for the struct. After all it is equivalent of an open handle for the block device so IMHO bdev_handle captures that better than bdev_ctx. Honza
On Mon, Jul 31, 2023 at 12:50:34PM +0200, Jan Kara wrote: > I think the bdev_handle name is fine for the struct. After all it is > equivalent of an open handle for the block device so IMHO bdev_handle > captures that better than bdev_ctx. Agreed.
diff --git a/block/bdev.c b/block/bdev.c index 979e28a46b98..c75de5cac2bc 100644 --- a/block/bdev.c +++ b/block/bdev.c @@ -846,6 +846,24 @@ struct block_device *blkdev_get_by_dev(dev_t dev, blk_mode_t mode, void *holder, } EXPORT_SYMBOL(blkdev_get_by_dev); +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); + handle->bdev = bdev; + handle->holder = holder; + return handle; +} +EXPORT_SYMBOL(blkdev_get_handle_by_dev); + /** * blkdev_get_by_path - open a block device by name * @path: path to the block device to open @@ -884,6 +902,28 @@ struct block_device *blkdev_get_by_path(const char *path, blk_mode_t mode, } EXPORT_SYMBOL(blkdev_get_by_path); +struct bdev_handle *blkdev_get_handle_by_path(const char *path, blk_mode_t mode, + void *holder, const struct blk_holder_ops *hops) +{ + struct bdev_handle *handle; + dev_t dev; + int error; + + error = lookup_bdev(path, &dev); + if (error) + return ERR_PTR(error); + + handle = blkdev_get_handle_by_dev(dev, mode, holder, hops); + if (!IS_ERR(handle) && (mode & BLK_OPEN_WRITE) && + bdev_read_only(handle->bdev)) { + blkdev_handle_put(handle); + return ERR_PTR(-EACCES); + } + + return handle; +} +EXPORT_SYMBOL(blkdev_get_handle_by_path); + void blkdev_put(struct block_device *bdev, void *holder) { struct gendisk *disk = bdev->bd_disk; @@ -920,6 +960,13 @@ void blkdev_put(struct block_device *bdev, void *holder) } EXPORT_SYMBOL(blkdev_put); +void blkdev_handle_put(struct bdev_handle *handle) +{ + blkdev_put(handle->bdev, handle->holder); + kfree(handle); +} +EXPORT_SYMBOL(blkdev_handle_put); + /** * lookup_bdev() - Look up a struct block_device by name. * @pathname: Name of the block device in the filesystem. diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index ed44a997f629..a910e9997ddd 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1471,14 +1471,24 @@ struct blk_holder_ops { #define sb_open_mode(flags) \ (BLK_OPEN_READ | (((flags) & SB_RDONLY) ? 0 : BLK_OPEN_WRITE)) +struct bdev_handle { + struct block_device *bdev; + void *holder; +}; + struct block_device *blkdev_get_by_dev(dev_t dev, blk_mode_t mode, void *holder, const struct blk_holder_ops *hops); struct block_device *blkdev_get_by_path(const char *path, blk_mode_t mode, void *holder, const struct blk_holder_ops *hops); +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 *blkdev_get_handle_by_path(const char *path, blk_mode_t mode, + void *holder, const struct blk_holder_ops *hops); int bd_prepare_to_claim(struct block_device *bdev, void *holder, const struct blk_holder_ops *hops); void bd_abort_claiming(struct block_device *bdev, void *holder); void blkdev_put(struct block_device *bdev, void *holder); +void blkdev_handle_put(struct bdev_handle *handle); /* just for blk-cgroup, don't use elsewhere */ struct block_device *blkdev_get_no_open(dev_t dev);
Create struct bdev_handle that contains all parameters that need to be passed to blkdev_put() and provide blkdev_get_handle_* functions that return this structure instead of plain bdev pointer. This will eventually allow us to pass one more argument to blkdev_put() without too much hassle. CC: Alasdair Kergon <agk@redhat.com> CC: Andrew Morton <akpm@linux-foundation.org> CC: Anna Schumaker <anna@kernel.org> CC: Chao Yu <chao@kernel.org> CC: Christian Borntraeger <borntraeger@linux.ibm.com> CC: Coly Li <colyli@suse.de CC: "Darrick J. Wong" <djwong@kernel.org> CC: Dave Kleikamp <shaggy@kernel.org> CC: David Sterba <dsterba@suse.com> CC: dm-devel@redhat.com CC: drbd-dev@lists.linbit.com CC: Gao Xiang <xiang@kernel.org> CC: Jack Wang <jinpu.wang@ionos.com> CC: Jaegeuk Kim <jaegeuk@kernel.org> CC: jfs-discussion@lists.sourceforge.net CC: Joern Engel <joern@lazybastard.org> CC: Joseph Qi <joseph.qi@linux.alibaba.com> CC: Kent Overstreet <kent.overstreet@gmail.com> CC: linux-bcache@vger.kernel.org CC: linux-btrfs@vger.kernel.org CC: linux-erofs@lists.ozlabs.org CC: <linux-ext4@vger.kernel.org> CC: linux-f2fs-devel@lists.sourceforge.net CC: linux-mm@kvack.org CC: linux-mtd@lists.infradead.org CC: linux-nfs@vger.kernel.org CC: linux-nilfs@vger.kernel.org CC: linux-nvme@lists.infradead.org CC: linux-pm@vger.kernel.org CC: linux-raid@vger.kernel.org CC: linux-s390@vger.kernel.org CC: linux-scsi@vger.kernel.org CC: linux-xfs@vger.kernel.org CC: "Md. Haris Iqbal" <haris.iqbal@ionos.com> CC: Mike Snitzer <snitzer@kernel.org> CC: Minchan Kim <minchan@kernel.org> CC: ocfs2-devel@oss.oracle.com CC: reiserfs-devel@vger.kernel.org CC: Sergey Senozhatsky <senozhatsky@chromium.org> CC: Song Liu <song@kernel.org> CC: Sven Schnelle <svens@linux.ibm.com> CC: target-devel@vger.kernel.org CC: Ted Tso <tytso@mit.edu> CC: Trond Myklebust <trond.myklebust@hammerspace.com> CC: xen-devel@lists.xenproject.org Signed-off-by: Jan Kara <jack@suse.cz> --- block/bdev.c | 47 ++++++++++++++++++++++++++++++++++++++++++ include/linux/blkdev.h | 10 +++++++++ 2 files changed, 57 insertions(+)