Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
On Wed 12-07-23 18:06:35, Haris Iqbal wrote: > On Thu, Jul 6, 2023 at 5:38 PM 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); > > +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 -- Jan Kara SUSE Labs, CR -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
On Thu, Jul 6, 2023 at 5:38 PM 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); +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) > > ? -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
On Thu, Jul 6, 2023 at 5:38 PM 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); > +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? > > ? > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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?
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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
--
Jan Kara
SUSE Labs, CR
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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-j...@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.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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
--
Jan Kara
SUSE Labs, CR
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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
>
--
Jan Kara
SUSE Labs, CR
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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 -- Jan Kara SUSE Labs, CR -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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 -- Jan Kara SUSE Labs, CR -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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); ? -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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, );
> + 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);
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 01/32] block: Provide blkdev_get_handle_* functions
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.
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
