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

[Christoph Hellwig <hch@lst.de>]

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