Re: [PATCH v2 01/34] bdev: open block device as files

Christoph Hellwig
2024-01-29T16:02:45Z

> +static unsigned blk_to_file_flags(blk_mode_t mode)
> +{
> + unsigned int flags = 0;
> +

...

> + /*
> + * O_EXCL is one of those flags that the VFS clears once it's done with
> + * the operation. So don't raise it here either.
> + */
> + if (mode & BLK_OPEN_NDELAY)
> + flags |= O_NDELAY;

O_EXCL isn't dealt with in this helper at all.

> + /*
> + * If BLK_OPEN_WRITE_IOCTL is set then this is a historical quirk
> + * associated with the floppy driver where it has allowed ioctls if the
> + * file was opened for writing, but does not allow reads or writes.
> + * Make sure that this quirk is reflected in @f_flags.
> + */
> + if (mode & BLK_OPEN_WRITE_IOCTL)
> + flags |= O_RDWR | O_WRONLY;

.. and BLK_OPEN_WRITE_IOCTL will never be passed to it. It only comes
from open block devices nodes.

That being said, passing BLK_OPEN_* to bdev_file_open_by_* actually
feels wrong. They deal with files and should just take normal
O_* flags instead of translating from BLK_OPEN_* to O_* back to
BLK_OPEN_* for the driver (where they make sense as the driver
flags are pretty different from what is passed to open).

Now of course changing that would make a mess of the whole series,
so maybe that can go into a new patch at the end?

> + * @noaccount: whether this is an internal open that shouldn't be counted
> */
> static struct file *alloc_file(const struct path *path, int flags,
> - const struct file_operations *fop)
> + const struct file_operations *fop, bool noaccount)

Just a suggestion as you are the maintainer here, but I always find
it hard to follow when infrastructure in subsystem A is changed in
a patch primarily changing subsystem B. Can the file_table.c
changes go into a separate patch or patches with commit logs
documenting their semantics?

And while we're at the semantics I find this area already a bit of a
a mess and this doesn't make it any better..

How about the following:

- alloc_file loses the actual file allocation and gets a new name
(unfortunatel init_file is already taken), callers call
alloc_empty_file_noaccount or alloc_empty_file plus the
new helper.
- similarly __alloc_file_pseudo is split into a helper creating
a path for mnt and inode, and callers call that plus the
file allocation

?

> +extern struct file *alloc_file_pseudo_noaccount(struct inode *, struct vfsmount *,

no need for the extern here.

> + struct block_device *s_bdev; /* can go away once we use an accessor for @s_bdev_file */

can you put the comment into a separate line to make it readable.

But I'm not even sure it should go away. s_bdev is used all over the
data and metadata I/O path, so caching it and avoiding multiple levels
of pointer chasing would seem useful.