> +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.
