Hi Amir,

How about this as a set of patches to do what you suggest[1] and hoist the
handler functions for I_OVL_INUSE into common code and rename the flag to
I_EXCL_INUSE.  This can then be shared with cachefiles - allowing me to get
rid of S_KERNEL_FILE.

I did split out the functionality for preventing file/dir removal to a
separate flag, I_NO_REMOVE, so that it's not tied to I_EXCL_INUSE in case
overlayfs doesn't want to use it.  The downside to that, though is that it
requires a separate locking of i_lock to set/clear it.

I also added four general tracepoints to log successful lock/unlock,
failure to lock and a bad unlock.  The lock tracepoints log which driver
asked for the lock and all tracepoints allow the driver to log an arbitrary
reference number (in cachefiles's case this is the object debug ID).

Questions:

 (1) Should it be using a flag in i_state or a flag in i_flags?  I'm not
     sure what the difference is really.

 (2) Do we really need to take i_lock when testing I_EXCL_INUSE?  Would
     READ_ONCE() suffice?


The patches are on a branch here:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=fscache-fixes

David

Link: https://lore.kernel.org/r/CAOQ4uxhRS3MGEnCUDcsB1RL0d1Oy0g0Rzm75hVFAJw2dJ7uKSA@mail.gmail.com/ [1]

---
David Howells (5):
      vfs, overlayfs, cachefiles: Turn I_OVL_INUSE into something generic
      vfs: Add tracepoints for inode_excl_inuse_trylock/unlock
      cachefiles: Split removal-prevention from S_KERNEL_FILE and extend effects
      cachefiles: Use I_EXCL_INUSE instead of S_KERNEL_FILE
      cachefiles: Remove the now-unused mark-inode-in-use tracepoints


 fs/cachefiles/namei.c             | 54 +++++++++++++-------------
 fs/inode.c                        | 56 +++++++++++++++++++++++++++
 fs/namei.c                        |  8 ++--
 fs/overlayfs/overlayfs.h          |  3 --
 fs/overlayfs/super.c              | 14 ++++---
 fs/overlayfs/util.c               | 43 ---------------------
 include/linux/fs.h                | 33 ++++++++++++++--
 include/trace/events/cachefiles.h | 63 -------------------------------
 8 files changed, 126 insertions(+), 148 deletions(-)

On Mon, Jan 31, 2022 at 5:12 PM David Howells <dhowells@redhat.com> wrote:
>
>
> Hi Amir,
>
> How about this as a set of patches to do what you suggest[1] and hoist the
> handler functions for I_OVL_INUSE into common code and rename the flag to
> I_EXCL_INUSE.  This can then be shared with cachefiles - allowing me to get
> rid of S_KERNEL_FILE.
>

They look like what I had in mind.
Unfortunately, I had forgotten about another use that ovl makes of the flag
(see comment on patch 1/5). I'd made a suggestion on how to get rid of that use
case, but I hope this won't complicate things too much for you.

> I did split out the functionality for preventing file/dir removal to a
> separate flag, I_NO_REMOVE, so that it's not tied to I_EXCL_INUSE in case
> overlayfs doesn't want to use it.  The downside to that, though is that it
> requires a separate locking of i_lock to set/clear it.
>
> I also added four general tracepoints to log successful lock/unlock,
> failure to lock and a bad unlock.  The lock tracepoints log which driver
> asked for the lock and all tracepoints allow the driver to log an arbitrary
> reference number (in cachefiles's case this is the object debug ID).
>
> Questions:
>
>  (1) Should it be using a flag in i_state or a flag in i_flags?  I'm not
>      sure what the difference is really.

Me neither.

>
>  (2) Do we really need to take i_lock when testing I_EXCL_INUSE?  Would
>      READ_ONCE() suffice?
>

For ovl_is_inuse() I think READ_ONCE() should suffice.

Thanks,
Amir.