On Mon, Jan 29, 2024 at 05:02:41PM +0100, Christoph Hellwig wrote:
> > +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.
Yeah, on purpose was my point bc we can just rely on @holder and passing
_EXCL without holder is invalid. But I could add it.
>
> > + /*
> > + * 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?
Yes, I had considered that and it would work but there's the issue that
we need to figure out how to handle BLK_OPEN_RESTRICT_WRITES. It has no
corresponding O_* flag that would let us indicate this. So I had
considered:
1/ Expose bdev_file_open_excl() so callers don't need to pass any
specific flags. Nearly all filesystems would effectively use this
helper as sb_open_mode() adds it implicitly. That would have the
side-effect of introducing another open helper ofc; possibly two if
we take _by_dev() and _by_path() into account.
2/ Abuse an O_* flag to mean BLK_OPEN_RESTRICT_WRITES. For example,
O_TRUNC or O_NOCTTY which is pretty yucky.
3/ Introduce an internal O_* flag which is also ugly. Vomitorious and my
co-maintainers would likely chop off my hands so I can't go near a
computer again.
3/ Make O_EXCL when passed together with bdev_file_open_by_*() always
imply BLK_OPEN_RESTRICT_WRITES.
The 3/ option would probably be the cleanest one and I think that all
filesystems now pass at least a holder and holder ops so this _should_
work.
Thoughts?
>
> > + * @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
>
> ?
Ok, let me see how far I get.
>
> > +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.
Fair.
