Messages in this thread | | | Date | Mon, 13 Jan 2020 16:35:21 -0800 | From | Ira Weiny <> | Subject | Re: [RFC PATCH V2 08/12] fs/xfs: Add lock/unlock mode to xfs |
| |
On Mon, Jan 13, 2020 at 02:19:57PM -0800, Darrick J. Wong wrote: > On Fri, Jan 10, 2020 at 11:29:38AM -0800, ira.weiny@intel.com wrote: > > From: Ira Weiny <ira.weiny@intel.com>
[snip]
> > > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c > > index 401da197f012..e8fd95b75e5b 100644 > > --- a/fs/xfs/xfs_inode.c > > +++ b/fs/xfs/xfs_inode.c > > @@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared( > > * > > * Basic locking order: > > * > > - * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock > > + * i_rwsem -> i_dax_sem -> i_mmap_lock -> page_lock -> i_ilock > > Mmmmmm, more locks. Can we skip the extra lock if CONFIG_FSDAX=n or if > the filesystem devices don't support DAX at all?
I'll look into it.
> > Also, I don't think we're actually following the i_rwsem -> i_daxsem > order in fallocate, and possibly elsewhere too?
I'll have to verify. It took a lot of iterations to get the order working so I'm not going to claim perfection.
> > Does the vfs have to take the i_dax_sem to do remapping things like > reflink? (Pretend that reflink and dax are compatible for the moment)
Honestly I can't say for sure. For this series I was careful to exclude reflink from the locking requirement.
[snip]
> > > > #define XFS_LOCK_FLAGS \ > > { XFS_IOLOCK_EXCL, "IOLOCK_EXCL" }, \ > > @@ -289,7 +295,9 @@ static inline void xfs_ifunlock(struct xfs_inode *ip) > > { XFS_ILOCK_EXCL, "ILOCK_EXCL" }, \ > > { XFS_ILOCK_SHARED, "ILOCK_SHARED" }, \ > > { XFS_MMAPLOCK_EXCL, "MMAPLOCK_EXCL" }, \ > > - { XFS_MMAPLOCK_SHARED, "MMAPLOCK_SHARED" } > > + { XFS_MMAPLOCK_SHARED, "MMAPLOCK_SHARED" }, \ > > + { XFS_DAX_EXCL, "DAX_EXCL" }, \ > > Whitespace between the comma & string.
Fixed.
[snip]
> > + > > static const struct inode_operations xfs_dir_inode_operations = { > > .create = xfs_vn_create, > > .lookup = xfs_vn_lookup, > > @@ -1372,7 +1394,7 @@ xfs_setup_iops( > > > > switch (inode->i_mode & S_IFMT) { > > case S_IFREG: > > - inode->i_op = &xfs_inode_operations; > > + inode->i_op = &xfs_reg_inode_operations; > > xfs_file_inode_operations?
Sounds better. Fixed.
Ira
| |