| Message ID | 20230704122224.16257-1-jack@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | block: Make blkdev_get_by_*() return handle | expand |
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, 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 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(+)