On Sun, May 26, 2024 at 03:16:37PM -0700, Linus Torvalds wrote:
> Hopefully in most cases the compiler sees the previous test for
> fd.file, realizes the new test is unnecessary and optimizes it away.
It does.
> Except we most definitely pass around 'struct fd *' in some places (at
> least ovlfs), so I doubt that will be the case everywhere.
... and those places need care, really. I've done that review
quite recently. I don't think you'd been on Cc in related thread...
<checks> nope.
|| Another is overlayfs ovl_real_fdget() and ovl_real_fdget_meta().
|| Those two are arguably too clever for their readers' good.
|| It's still "borrow or clone, and remember which one had it been",
|| but that's mixed with returning errors:
|| err = ovl_read_fdget(file, &fd);
|| may construct and store a junk value in fd if err is non-zero.
|| Not a bug, but only because all callers remember to ignore that
|| value in such case.
Junk value as in e.g. <ERR_PTR(-EIO), FDPUT_FPUT>; it's trivial
to avoid (
file = ovl_open_realfile(file, &realpath);
if (IS_ERR(file))
return ERR_PTR(file);
real->flags = FDPUT_FPUT;
real->file = file;
return 0;
instead of
real->flags = FDPUT_FPUT;
real->file = ovl_open_realfile(file, &realpath);
return PTR_ERR_OR_ZERO(real->file);
we have there right now and similar for <ERR_PTR(-EPERM), 0> pairs),
but note that anything like your switch to struct-wrapped unsigned long
would need similar massage there anyway.
> What would make more sense is if you make the "fd_empty()" test be
> about the _flags_, and then both the fp_empty() test and the test
> inside fdput() would be testing the same things.
Umm... What encoding would you use?
> Sadly, we'd need another bit in the flags. One option is easy enough -
> we'd just have to make 'struct file' always be 8-byte aligned, which
> it effectively always is.
>
> Or we'd need to make the rule be that FDPUT_POS_UNLOCK only gets set
> if FDPUT_FPUT is set.
Huh? That makes no sense - open a file, fork, and there you go;
same file reference in unshared descriptor tables in child and parent.
You definitely don't want to bump refcount on fdget_pos() in either
and you don't want to lose read/write/lseek serialization.
FDPUT_FPUT is *not* a property of file; it's about the original
reference to file not being guaranteed to stay pinned for the lifetime
of struct fd in question...
