Re: [PATCH v2 29/34] bdev: make struct bdev_handle private to the block layer

Christian Brauner
2024-02-01T15:08:03Z

> > * Use this interface ONLY if you really do not have anything better - i.e. when
> > * you are behind a truly sucky interface and all you are given is a device
> ^^^
> I guess this part of comment is stale now?

Indeed, I removed that.

>
> > @@ -902,7 +897,22 @@ struct bdev_handle *bdev_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
> > handle->bdev = bdev;
> > handle->holder = holder;
> > handle->mode = mode;
> > - return handle;
> > +
> > + /*
> > + * Preserve backwards compatibility and allow large file access
> > + * even if userspace doesn't ask for it explicitly. Some mkfs
> > + * binary needs it. We might want to drop this workaround
> > + * during an unstable branch.
> > + */
>
> Heh, I think the sentense "We might want to drop this workaround during an
> unstable branch." does not need to be moved as well :)

Dropped.

> > - handle = bdev_open_by_dev(dev, mode, holder, hops);
> > - if (IS_ERR(handle))
> > - return ERR_CAST(handle);
> > + ret = bdev_permission(dev, 0, holder);
> ^^ Maybe I'm missing something but why
> do you pass 0 mode here?

Lack of caffeine? Fixed. Thanks for catching that.

>
>
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + bdev = blkdev_get_no_open(dev);
> > + if (!bdev)
> > + return ERR_PTR(-ENXIO);
> >
> > flags = blk_to_file_flags(mode);
> > - bdev_file = alloc_file_pseudo_noaccount(handle->bdev->bd_inode,
> > + bdev_file = alloc_file_pseudo_noaccount(bdev->bd_inode,
> > blockdev_mnt, "", flags | O_LARGEFILE, &def_blk_fops);
> > if (IS_ERR(bdev_file)) {
> > - bdev_release(handle);
> > + blkdev_put_no_open(bdev);
> > return bdev_file;
> > }
> > - ihold(handle->bdev->bd_inode);
> > + bdev_file->f_mode &= ~FMODE_OPENED;
>
> Hum, why do you need these games with FMODE_OPENED? I suspect you want to
> influence fput() behavior but then AFAICT we will leak dentry, mnt, etc. on
> error? If this is indeed needed, it deserves a comment...

I rewrote this.

Total diff I applied is:

diff --git a/block/bdev.c b/block/bdev.c
index 0e8984884236..ba9dfa4648ca 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -893,12 +893,6 @@ int bdev_open(struct block_device *bdev, blk_mode_t mode, void *holder,
handle->holder = holder;
handle->mode = mode;

- /*
- * Preserve backwards compatibility and allow large file access
- * even if userspace doesn't ask for it explicitly. Some mkfs
- * binary needs it. We might want to drop this workaround
- * during an unstable branch.
- */
bdev_file->f_flags |= O_LARGEFILE;
bdev_file->f_mode |= FMODE_BUF_RASYNC | FMODE_CAN_ODIRECT;
if (bdev_nowait(bdev))
@@ -960,7 +954,7 @@ struct file *bdev_file_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
unsigned int flags;
int ret;

- ret = bdev_permission(dev, 0, holder);
+ ret = bdev_permission(dev, mode, holder);
if (ret)
return ERR_PTR(ret);

@@ -975,16 +969,14 @@ struct file *bdev_file_open_by_dev(dev_t dev, blk_mode_t mode, void *holder,
blkdev_put_no_open(bdev);
return bdev_file;
}
- bdev_file->f_mode &= ~FMODE_OPENED;
-
ihold(bdev->bd_inode);
+
ret = bdev_open(bdev, mode, holder, hops, bdev_file);
if (ret) {
+ blkdev_put_no_open(bdev);
fput(bdev_file);
return ERR_PTR(ret);
}
- /* Now that thing is opened. */
- bdev_file->f_mode |= FMODE_OPENED;
return bdev_file;
}
EXPORT_SYMBOL(bdev_file_open_by_dev);
diff --git a/block/fops.c b/block/fops.c
index 81ff8c0ce32f..a1ba1a50ae77 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -622,7 +622,8 @@ static int blkdev_open(struct inode *inode, struct file *filp)

static int blkdev_release(struct inode *inode, struct file *filp)
{
- bdev_release(filp->private_data);
+ if (filp->private_data)
+ bdev_release(filp->private_data);
return 0;
}