On Thu, Jul 25, 2024 at 11:19:33PM +0300, Amir Goldstein wrote:
> On Thu, Jul 25, 2024 at 9:20 PM Josef Bacik <josef@xxxxxxxxxxxxxx> wrote:
> >
> > FS_PRE_ACCESS or FS_PRE_MODIFY will be generated on page fault depending
> > on the faulting method.
> >
> > This pre-content event is meant to be used by hierarchical storage
> > managers that want to fill in the file content on first read access.
> >
> > Signed-off-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > ---
> > fs/notify/fsnotify.c | 13 +++++++++
> > include/linux/fsnotify_backend.h | 14 +++++++++
> > mm/filemap.c | 50 ++++++++++++++++++++++++++++----
> > 3 files changed, 71 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> > index 1ca4a8da7f29..435232d46b4f 100644
> > --- a/fs/notify/fsnotify.c
> > +++ b/fs/notify/fsnotify.c
> > @@ -28,6 +28,19 @@ void __fsnotify_vfsmount_delete(struct vfsmount *mnt)
> > fsnotify_clear_marks_by_mount(mnt);
> > }
> >
> > +bool fsnotify_file_has_content_watches(struct file *file)
>
> nit: has_pre_content_watches...
>
> > +{
> > + struct inode *inode = file_inode(file);
> > + struct super_block *sb = inode->i_sb;
> > + struct mount *mnt = real_mount(file->f_path.mnt);
> > + u32 mask = inode->i_fsnotify_mask;
> > +
> > + mask |= mnt->mnt_fsnotify_mask;
> > + mask |= sb->s_fsnotify_mask;
> > +
> > + return !!(mask & FSNOTIFY_PRE_CONTENT_EVENTS);
>
> This can use the fsnotify_object_watched() helper, and it will need
> the READ_ONCE() that are just being added to avoid data races.
>
> > +}
> > +
> > /**
> > * fsnotify_unmount_inodes - an sb is unmounting. handle any watched inodes.
> > * @sb: superblock being unmounted.
> > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> > index 36c3d18cc40a..6983fbf096b8 100644
> > --- a/include/linux/fsnotify_backend.h
> > +++ b/include/linux/fsnotify_backend.h
> > @@ -900,6 +900,15 @@ static inline void fsnotify_init_event(struct fsnotify_event *event)
> > INIT_LIST_HEAD(&event->list);
> > }
> >
> > +#ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
> > +bool fsnotify_file_has_content_watches(struct file *file);
> > +#else
> > +static inline bool fsnotify_file_has_content_watches(struct file *file)
> > +{
> > + return false;
> > +}
> > +#endif /* CONFIG_FANOTIFY_ACCESS_PERMISSIONS */
> > +
> > #else
> >
> > static inline int fsnotify(__u32 mask, const void *data, int data_type,
> > @@ -938,6 +947,11 @@ static inline u32 fsnotify_get_cookie(void)
> > static inline void fsnotify_unmount_inodes(struct super_block *sb)
> > {}
> >
> > +static inline bool fsnotify_file_has_content_watches(struct file *file)
> > +{
> > + return false;
> > +}
> > +
> > #endif /* CONFIG_FSNOTIFY */
> >
> > #endif /* __KERNEL __ */
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index ca8c8d889eef..cc9d7885bbe3 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -46,6 +46,7 @@
> > #include <linux/pipe_fs_i.h>
> > #include <linux/splice.h>
> > #include <linux/rcupdate_wait.h>
> > +#include <linux/fsnotify.h>
> > #include <asm/pgalloc.h>
> > #include <asm/tlbflush.h>
> > #include "internal.h"
> > @@ -3112,13 +3113,13 @@ static int lock_folio_maybe_drop_mmap(struct vm_fault *vmf, struct folio *folio,
> > * that. If we didn't pin a file then we return NULL. The file that is
> > * returned needs to be fput()'ed when we're done with it.
> > */
> > -static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> > +static struct file *do_sync_mmap_readahead(struct vm_fault *vmf,
> > + struct file *fpin)
> > {
> > struct file *file = vmf->vma->vm_file;
> > struct file_ra_state *ra = &file->f_ra;
> > struct address_space *mapping = file->f_mapping;
> > DEFINE_READAHEAD(ractl, file, ra, mapping, vmf->pgoff);
> > - struct file *fpin = NULL;
> > unsigned long vm_flags = vmf->vma->vm_flags;
> > unsigned int mmap_miss;
> >
> > @@ -3182,12 +3183,12 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
> > * was pinned if we have to drop the mmap_lock in order to do IO.
> > */
> > static struct file *do_async_mmap_readahead(struct vm_fault *vmf,
> > - struct folio *folio)
> > + struct folio *folio,
> > + struct file *fpin)
> > {
>
> If I am reading correctly, iomap (i.e. xfs) write shared memory fault
> does not reach this code?
>
> Do we care about writable shared memory faults use case for HSM?
> It does not sound very relevant to HSM, but we cannot just ignore it..
>
Sorry I realized I went off to try and solve this problem and never responded to
you. I'm addressing the other comments, but this one is a little tricky.
We're kind of stuck between a rock and a hard place with this. I had originally
put this before the ->fault() callback, but purposefully moved it into
filemap_fault() because I want to be able to drop the mmap lock while we're
waiting for a response from the HSM.
The reason to do this is because there are things that take the mmap lock for
simple things outside of the process, like /proc/$PID/smaps and other related
things, and this can cause high priority tasks to block behind possibly low
priority IO, creating a priority inversion.
Now, I'm not sure how widespread of a problem this is anymore, I know there's
been work done to the kernel and tools to avoid this style of problem. I'm ok
with a "try it and see" approach, but I don't love that.
However I think putting fsnotify hooks into XFS itself for this particular path
is a good choice either. What do you think? Just move it to before ->fault(),
leave the mmap lock in place, and be done with it? Thanks,
Josef
