On Tue 06-02-24 14:39:13, Christian Brauner wrote:
> On Mon, Feb 05, 2024 at 03:19:11PM +0100, Jan Kara wrote:
> > Hi!
> >
> > On Mon 05-02-24 12:55:18, Christian Brauner wrote:
> > > On Tue, Jan 23, 2024 at 02:26:17PM +0100, Christian Brauner wrote:
> > > > Hey Christoph,
> > > > Hey Jan,
> > > > Hey Jens,
> > > >
> > > > This opens block devices as files. Instead of introducing a separate
> > > > indirection into bdev_open_by_*() vis struct bdev_handle we can just
> > > > make bdev_file_open_by_*() return a struct file. Opening and closing a
> > > > block device from setup_bdev_super() and in all other places just
> > > > becomes equivalent to opening and closing a file.
> > > >
> > > > This has held up in xfstests and in blktests so far and it seems stable
> > > > and clean. The equivalence of opening and closing block devices to
> > > > regular files is a win in and of itself imho. Added to that is the
> > > > ability to do away with struct bdev_handle completely and make various
> > > > low-level helpers private to the block layer.
> > > >
> > > > All places were we currently stash a struct bdev_handle we just stash a
> > > > file and use an accessor such as file_bdev() akin to I_BDEV() to get to
> > > > the block device.
> > > >
> > > > It's now also possible to use file->f_mapping as a replacement for
> > > > bdev->bd_inode->i_mapping and file->f_inode or file->f_mapping->host as
> > > > an alternative to bdev->bd_inode allowing us to significantly reduce or
> > > > even fully remove bdev->bd_inode in follow-up patches.
> > > >
> > > > In addition, we could get rid of sb->s_bdev and various other places
> > > > that stash the block device directly and instead stash the block device
> > > > file. Again, this is follow-up work.
> > > >
> > > > Thanks!
> > > > Christian
> > > >
> > > > Signed-off-by: Christian Brauner <brauner@xxxxxxxxxx>
> > > > ---
> > >
> > > With all fixes applied I've moved this into vfs.super on vfs/vfs.git so
> > > this gets some exposure in -next asap. Please let me know if you have
> > > quarrels with that.
> >
> > No quarrels really. I went through the patches and all of them look fine to
> > me to feel free to add:
> >
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
> >
> > I have just noticed that in "bdev: make struct bdev_handle private to the
> > block layer" in bdev_open() we are still leaking the handle in case of
> > error. This is however temporary (until the end of the series when we get
> > rid of handles altogether) so whatever.
>
> Can you double-check what's in vfs.super right now? I thought I fixed
> this up. I'll check too!
Well, you've fixed the "double allocation" issue but there's still a
problem that you do:
int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
const struct blk_holder_ops *hops, struct file *bdev_file)
{
...
handle = kmalloc(sizeof(struct bdev_handle), GFP_KERNEL);
if (!handle)
return -ENOMEM;
if (holder) {
mode |= BLK_OPEN_EXCL;
ret = bd_prepare_to_claim(bdev, holder, hops);
if (ret)
return ret;
} else {
...
So in case bd_prepare_to_claim() fails we forget to free the allocated
handle.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
