On Fri, Feb 21, 2020 at 04:33:17PM -0800, Darrick J. Wong wrote:
> On Thu, Feb 20, 2020 at 04:41:28PM -0800, ira.weiny@xxxxxxxxx wrote:
> > From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > DAX requires special address space operations (aops). Changing DAX
> > state therefore requires changing those aops.
> >
> > However, many functions require aops to remain consistent through a deep
> > call stack.
> >
> > Define a vfs level inode rwsem to protect aops throughout call stacks
> > which require them.
> >
> > Finally, define calls to be used in subsequent patches when aops usage
> > needs to be quiesced by the file system.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> >
> > ---
> > Changes from V3:
> > Convert from global to a per-inode rwsem
> > Remove pre-optimization
> > Remove static branch stuff
> > Change function names from inode_dax_state_* to inode_aops_*
> > I kept 'inode' as the synchronization is at the inode
> > level now (probably where it belongs)...
> >
> > and I still prefer *_[down|up]_[read|write] as those
> > names better reflect the use and interaction between
> > users (readers) and writers better. 'XX_start_aop()'
> > would have to be matched with something like
> > 'XX_wait_for_aop_user()' and 'XX_release_aop_users()' or
> > something which does not make sense on the 'writer'
> > side.
> >
> > Changes from V2
> >
> > Rebase on linux-next-08-02-2020
> >
> > Fix locking order
> > Change all references from mode to state where appropriate
> > add CONFIG_FS_DAX requirement for state change
> > Use a static branch to enable locking only when a dax capable
> > device has been seen.
> >
> > Move the lock to a global vfs lock
> >
> > this does a few things
> > 1) preps us better for ext4 support
> > 2) removes funky callbacks from inode ops
> > 3) remove complexity from XFS and probably from
> > ext4 later
> >
> > We can do this because
> > 1) the locking order is required to be at the
> > highest level anyway, so why complicate xfs
> > 2) We had to move the sem to the super_block
> > because it is too heavy for the inode.
> > 3) After internal discussions with Dan we
> > decided that this would be easier, just as
> > performant, and with slightly less overhead
> > than in the VFS SB.
> >
> > We also change the functions names to up/down;
> > read/write as appropriate. Previous names were over
> > simplified.
> >
> > Update comments and documentation
> >
> > squash: add locking
> > ---
> > Documentation/filesystems/vfs.rst | 16 ++++++++
> > fs/attr.c | 1 +
> > fs/inode.c | 15 +++++--
> > fs/iomap/buffered-io.c | 1 +
> > fs/open.c | 4 ++
> > fs/stat.c | 2 +
> > fs/xfs/xfs_icache.c | 1 +
> > include/linux/fs.h | 66 ++++++++++++++++++++++++++++++-
> > mm/fadvise.c | 7 +++-
> > mm/filemap.c | 4 ++
> > mm/huge_memory.c | 1 +
> > mm/khugepaged.c | 2 +
> > mm/util.c | 9 ++++-
> > 13 files changed, 121 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> > index 7d4d09dd5e6d..4a10a232f8e2 100644
> > --- a/Documentation/filesystems/vfs.rst
> > +++ b/Documentation/filesystems/vfs.rst
> > @@ -934,6 +934,22 @@ cache in your filesystem. The following members are defined:
> > Called during swapoff on files where swap_activate was
> > successful.
> >
> > +Changing DAX 'state' dynamically
> > +----------------------------------
> > +
> > +Some file systems which support DAX want to be able to change the DAX state
> > +dyanically. To switch the state safely we lock the inode state in all "normal"
> > +file system operations and restrict state changes to those operations. The
> > +specific rules are.
> > +
> > + 1) the direct_IO address_space_operation must be supported in all
> > + potential a_ops vectors for any state suported by the inode.
> > +
> > + 3) DAX state changes shall not be allowed while the file is mmap'ed
> > + 4) For non-mmaped operations the VFS layer must take the read lock for any
> > + use of IS_DAX()
> > + 5) Filesystems take the write lock when changing DAX states.
> > +
> >
> > The File Object
> > ===============
> > diff --git a/fs/attr.c b/fs/attr.c
> > index b4bbdbd4c8ca..9b15f73d1079 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -332,6 +332,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
> > if (error)
> > return error;
> >
> > + /* DAX read state should already be held here */
> > if (inode->i_op->setattr)
> > error = inode->i_op->setattr(dentry, attr);
> > else
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 7d57068b6b7a..6e4f1cc872f2 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -200,6 +200,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
> > #endif
> > inode->i_flctx = NULL;
> > this_cpu_inc(nr_inodes);
> > + init_rwsem(&inode->i_aops_sem);
> >
> > return 0;
> > out:
> > @@ -1616,11 +1617,19 @@ EXPORT_SYMBOL(iput);
> > */
> > int bmap(struct inode *inode, sector_t *block)
> > {
> > - if (!inode->i_mapping->a_ops->bmap)
> > - return -EINVAL;
> > + int ret = 0;
> > +
> > + inode_aops_down_read(inode);
> > + if (!inode->i_mapping->a_ops->bmap) {
> > + ret = -EINVAL;
> > + goto err;
> > + }
> >
> > *block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
> > - return 0;
> > +
> > +err:
> > + inode_aops_up_read(inode);
> > + return ret;
> > }
> > EXPORT_SYMBOL(bmap);
> > #endif
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index 7c84c4c027c4..e313a34d5fa6 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -999,6 +999,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count,
> > offset = offset_in_page(pos);
> > bytes = min_t(loff_t, PAGE_SIZE - offset, count);
> >
> > + /* DAX state read should already be held here */
> > if (IS_DAX(inode))
> > status = iomap_dax_zero(pos, offset, bytes, iomap);
> > else
> > diff --git a/fs/open.c b/fs/open.c
> > index 0788b3715731..3abf0bfac462 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -59,10 +59,12 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> > if (ret)
> > newattrs.ia_valid |= ret | ATTR_FORCE;
> >
> > + inode_aops_down_read(dentry->d_inode);
> > inode_lock(dentry->d_inode);
> > /* Note any delegations or leases have already been broken: */
> > ret = notify_change(dentry, &newattrs, NULL);
> > inode_unlock(dentry->d_inode);
> > + inode_aops_up_read(dentry->d_inode);
> > return ret;
> > }
> >
> > @@ -306,7 +308,9 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
> > return -EOPNOTSUPP;
> >
> > file_start_write(file);
> > + inode_aops_down_read(inode);
> > ret = file->f_op->fallocate(file, mode, offset, len);
> > + inode_aops_up_read(inode);
> >
> > /*
> > * Create inotify and fanotify events.
> > diff --git a/fs/stat.c b/fs/stat.c
> > index 894699c74dde..274b3ccc82b1 100644
> > --- a/fs/stat.c
> > +++ b/fs/stat.c
> > @@ -79,8 +79,10 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
> > if (IS_AUTOMOUNT(inode))
> > stat->attributes |= STATX_ATTR_AUTOMOUNT;
> >
> > + inode_aops_down_read(inode);
> > if (IS_DAX(inode))
> > stat->attributes |= STATX_ATTR_DAX;
> > + inode_aops_up_read(inode);
> >
> > if (inode->i_op->getattr)
> > return inode->i_op->getattr(path, stat, request_mask,
> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index 836a1f09be03..3e83a97dc047 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -420,6 +420,7 @@ xfs_iget_cache_hit(
> > rcu_read_unlock();
> >
> > ASSERT(!rwsem_is_locked(&inode->i_rwsem));
> > + ASSERT(!rwsem_is_locked(&inode->i_aops_sem));
> > error = xfs_reinit_inode(mp, inode);
> > if (error) {
> > bool wake;
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 63d1e533a07d..ad0f2368031b 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -40,6 +40,7 @@
> > #include <linux/fs_types.h>
> > #include <linux/build_bug.h>
> > #include <linux/stddef.h>
> > +#include <linux/percpu-rwsem.h>
> >
> > #include <asm/byteorder.h>
> > #include <uapi/linux/fs.h>
> > @@ -359,6 +360,11 @@ typedef struct {
> > typedef int (*read_actor_t)(read_descriptor_t *, struct page *,
> > unsigned long, unsigned long);
> >
> > +/**
> > + * NOTE: DO NOT define new functions in address_space_operations without first
> > + * considering how dynamic DAX states are to be supported. See the
> > + * inode_aops_*_read() functions
> > + */
> > struct address_space_operations {
> > int (*writepage)(struct page *page, struct writeback_control *wbc);
> > int (*readpage)(struct file *, struct page *);
> > @@ -735,6 +741,9 @@ struct inode {
> > #endif
> >
> > void *i_private; /* fs or device private pointer */
> > +#if defined(CONFIG_FS_DAX)
> > + struct rw_semaphore i_aops_sem;
> > +#endif
> > } __randomize_layout;
> >
> > struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode);
> > @@ -1817,6 +1826,11 @@ struct block_device_operations;
> >
> > struct iov_iter;
> >
> > +/**
> > + * NOTE: DO NOT define new functions in file_operations without first
> > + * considering how dynamic address_space operations are to be supported. See
> > + * the inode_aops_*_read() functions in this file.
> > + */
> > struct file_operations {
> > struct module *owner;
> > loff_t (*llseek) (struct file *, loff_t, int);
> > @@ -1889,16 +1903,64 @@ struct inode_operations {
> > int (*set_acl)(struct inode *, struct posix_acl *, int);
> > } ____cacheline_aligned;
> >
> > +#if defined(CONFIG_FS_DAX)
> > +/*
> > + * Filesystems wishing to support dynamic DAX states must do the following.
> > + *
> > + * 1) the direct_IO address_space_operation must be supported in all potential
> > + * a_ops vectors for any state suported by the inode. This is because the
> > + * direct_IO function is used as a flag long before the function is called.
> > +
> > + * 3) DAX state changes shall not be allowed while the file is mmap'ed
> > + * 4) For non-mmaped operations the VFS layer must take the read lock for any
> > + * use of IS_DAX()
> > + * 5) Filesystems take the write lock when changing DAX states.
>
> Do you envision ext4 migrating their aops changing code to use
> i_aops_sem in the future?
Ah... yea, which, at that time, this comment would be completely wrong. I
tried to remove references to dax as per Dave but I missed this one.
Thanks,
Ira
>
> --D
>
> > + */
> > +static inline void inode_aops_down_read(struct inode *inode)
> > +{
> > + down_read(&inode->i_aops_sem);
> > +}
> > +static inline void inode_aops_up_read(struct inode *inode)
> > +{
> > + up_read(&inode->i_aops_sem);
> > +}
> > +static inline void inode_aops_down_write(struct inode *inode)
> > +{
> > + down_write(&inode->i_aops_sem);
> > +}
> > +static inline void inode_aops_up_write(struct inode *inode)
> > +{
> > + up_write(&inode->i_aops_sem);
> > +}
> > +#else /* !CONFIG_FS_DAX */
> > +#define inode_aops_down_read(inode) do { (void)(inode); } while (0)
> > +#define inode_aops_up_read(inode) do { (void)(inode); } while (0)
> > +#define inode_aops_down_write(inode) do { (void)(inode); } while (0)
> > +#define inode_aops_up_write(inode) do { (void)(inode); } while (0)
> > +#endif /* CONFIG_FS_DAX */
> > +
> > static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
> > struct iov_iter *iter)
> > {
> > - return file->f_op->read_iter(kio, iter);
> > + struct inode *inode = file_inode(kio->ki_filp);
> > + ssize_t ret;
> > +
> > + inode_aops_down_read(inode);
> > + ret = file->f_op->read_iter(kio, iter);
> > + inode_aops_up_read(inode);
> > + return ret;
> > }
> >
> > static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
> > struct iov_iter *iter)
> > {
> > - return file->f_op->write_iter(kio, iter);
> > + struct inode *inode = file_inode(kio->ki_filp);
> > + ssize_t ret;
> > +
> > + inode_aops_down_read(inode);
> > + ret = file->f_op->write_iter(kio, iter);
> > + inode_aops_up_read(inode);
> > + return ret;
> > }
> >
> > static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> > diff --git a/mm/fadvise.c b/mm/fadvise.c
> > index 4f17c83db575..6a30febb11e0 100644
> > --- a/mm/fadvise.c
> > +++ b/mm/fadvise.c
> > @@ -48,6 +48,8 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> > bdi = inode_to_bdi(mapping->host);
> >
> > if (IS_DAX(inode) || (bdi == &noop_backing_dev_info)) {
> > + int ret = 0;
> > +
> > switch (advice) {
> > case POSIX_FADV_NORMAL:
> > case POSIX_FADV_RANDOM:
> > @@ -58,9 +60,10 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> > /* no bad return value, but ignore advice */
> > break;
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > }
> > - return 0;
> > +
> > + return ret;
> > }
> >
> > /*
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 1784478270e1..3a7863ba51b9 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -2293,6 +2293,8 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> > * and return. Otherwise fallthrough to buffered io for
> > * the rest of the read. Buffered reads will not work for
> > * DAX files, so don't bother trying.
> > + *
> > + * IS_DAX is protected under ->read_iter lock
> > */
> > if (retval < 0 || !count || iocb->ki_pos >= size ||
> > IS_DAX(inode))
> > @@ -3377,6 +3379,8 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> > * holes, for example. For DAX files, a buffered write will
> > * not succeed (even if it did, DAX does not handle dirty
> > * page-cache pages correctly).
> > + *
> > + * IS_DAX is protected under ->write_iter lock
> > */
> > if (written < 0 || !iov_iter_count(from) || IS_DAX(inode))
> > goto out;
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index b08b199f9a11..3d05bd10d83e 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -572,6 +572,7 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
> > unsigned long ret;
> > loff_t off = (loff_t)pgoff << PAGE_SHIFT;
> >
> > + /* Should not need locking here because mmap is not allowed */
> > if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD))
> > goto out;
> >
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > index b679908743cb..f048178e2b93 100644
> > --- a/mm/khugepaged.c
> > +++ b/mm/khugepaged.c
> > @@ -1592,9 +1592,11 @@ static void collapse_file(struct mm_struct *mm,
> > } else { /* !is_shmem */
> > if (!page || xa_is_value(page)) {
> > xas_unlock_irq(&xas);
> > + inode_aops_down_read(file->f_inode);
> > page_cache_sync_readahead(mapping, &file->f_ra,
> > file, index,
> > PAGE_SIZE);
> > + inode_aops_up_read(file->f_inode);
> > /* drain pagevecs to help isolate_lru_page() */
> > lru_add_drain();
> > page = find_lock_page(mapping, index);
> > diff --git a/mm/util.c b/mm/util.c
> > index 988d11e6c17c..a4fb0670137d 100644
> > --- a/mm/util.c
> > +++ b/mm/util.c
> > @@ -501,11 +501,18 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
> >
> > ret = security_mmap_file(file, prot, flag);
> > if (!ret) {
> > - if (down_write_killable(&mm->mmap_sem))
> > + if (file)
> > + inode_aops_down_read(file_inode(file));
> > + if (down_write_killable(&mm->mmap_sem)) {
> > + if (file)
> > + inode_aops_up_read(file_inode(file));
> > return -EINTR;
> > + }
> > ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,
> > &populate, &uf);
> > up_write(&mm->mmap_sem);
> > + if (file)
> > + inode_aops_up_read(file_inode(file));
> > userfaultfd_unmap_complete(mm, &uf);
> > if (populate)
> > mm_populate(ret, populate);
> > --
> > 2.21.0
> >
