On Mon, Jul 31, 2017 at 11:25:34AM -0700, Dan Williams wrote:
> On Mon, Jul 31, 2017 at 10:09 AM, Darrick J. Wong
> <darrick.wong@xxxxxxxxxx> wrote:
> >> index 93e955262d07..c4fc79a0704f 100644
> >> --- a/fs/xfs/xfs_bmap_util.c
> >> +++ b/fs/xfs/xfs_bmap_util.c
> >> @@ -1387,6 +1387,92 @@ xfs_zero_file_space(
> >>
> >> }
> >>
> >> +int
> >> +xfs_seal_file_space(
> >> + struct xfs_inode *ip,
> >> + xfs_off_t offset,
> >> + xfs_off_t len)
> >> +{
> >> + struct inode *inode = VFS_I(ip);
> >> + struct address_space *mapping = inode->i_mapping;
> >> + int error = 0;
> >> +
> >> + if (offset)
> >> + return -EINVAL;
> >> +
> >> + i_mmap_lock_read(mapping);
> >
> > (Are we allowed to take address_space->i_mmap_rwsem while holding
> > xfs_inode->i_mmaplock?)
>
> Empirically, yes. Lockdep complains when those locks are taken in the
> reverse order.
My pet hate: people who rely on lockdep to tell them that locking is
wrong rather than understanding what the correct locking order is
before writing code.
> That seems to be inconsistent with the "mmap_sem -> i_mmap_lock ->
> page_lock" note in the xfs_ilock comment. Am I confusing what
> i_mmap_lock means in that comment, is that the i_mmap_lock_read(), or
> the i_mmaplock in the xfs_inode?
The latter. The lock orders you need to pay attention to are
documented in mm/filemap.c. (Which, incidentally, needs updating to
refer to i_rwsem, not i_mutex.)
* ->i_mutex
* ->i_mmap_rwsem (truncate->unmap_mapping_range)
* ->mmap_sem
* ->i_mmap_rwsem
* ->page_table_lock or pte_lock (various, mainly in memory.c)
* ->mapping->tree_lock (arch-dependent flush_dcache_mmap_lock)
As it is, I think we shold not be taking internal mm/ state locks
deep inside filesystem code as it smells of layering violations. We
don't do this anywhere else for mapping checks - if we already hold
the XFS_MMAPLOCK_EXCL here, then we've already locked out page
faults from changing the state of the inode. In which case, we
should not need a mmap internal lock to be held here, same as all
the other filesystem operations that lock out page faults....
> >> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> >> + if (len == 0) {
> >> + /*
> >> + * Clear the immutable flag provided there are no active
> >> + * mappings. The active mapping check prevents an
> >> + * application that is assuming a static block map, for
> >> + * DAX or peer-to-peer DMA, from having this state
> >> + * silently change behind its back.
> >> + */
> >> + if (RB_EMPTY_ROOT(&mapping->i_mmap))
mapping_mapped(mapping)
> >> + inode->i_flags &= ~S_IOMAP_IMMUTABLE;
> >> + else
> >> + error = -EBUSY;
> >> + } else if (IS_IOMAP_IMMUTABLE(inode)) {
> >> + if (len == i_size_read(inode)) {
> >> + /*
> >> + * The file is already in the correct state,
> >> + * bail out without error below.
> >> + */
> >> + len = 0;
> >> + } else {
> >> + /* too late to allocate more space */
> >> + error = -ETXTBSY;
> >> + }
> >> + } else {
> >> + if (len < i_size_read(inode)) {
> >> + /*
> >> + * Since S_IOMAP_IMMUTABLE is inode global it
> >> + * does not make sense to fallocate(immutable)
> >> + * on a sub-range of the file.
> >> + */
> >> + error = -EINVAL;
> >> + } else if (!RB_EMPTY_ROOT(&mapping->i_mmap)) {
> >> + /*
> >> + * It's not strictly required to prevent setting
> >> + * immutable while a file is already mapped, but
> >> + * we do it for simplicity and symmetry with the
> >> + * S_IOMAP_IMMUTABLE disable case.
> >> + */
> >> + error = -EBUSY;
> >> + } else
> >> + inode->i_flags |= S_IOMAP_IMMUTABLE;
> >> + }
> >> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> >> + i_mmap_unlock_read(mapping);
> >> +
> >> + if (error || len == 0)
> >> + return error;
I have to say, I find this checking to be fairly grotty. The "len ==
0" API to remove the immutable flag is a gross hack. IMO, it's
better to add a separate fallocate command to "unseal" the extent
map, and let that happen according to whether the file is mapped or
not. Perhaps it would be better to start with a man page
documenting the desired API?
FWIW, the if/else if/else structure could be cleaned up with a
simple "goto out_unlock" construct such as:
/* don't make immutable if inode is currently mapped */
error = -EBUSY;
if (mapping_mapped(mapping))
goto out_unlock;
/* can't do anything if inode is already immutable */
error = -ETXTBSY;
if (IS_IMMUTABLE(inode) || IS_IOMAP_IMMUTABLE(inode))
goto out_unlock;
/* XFS only supports whole file extent immutability */
error = -EINVAL;
if (len != i_size_read(inode))
goto out_unlock;
/* all good to go */
error = 0;
out_unlock:
xfs_iunlock(ip, XFS_ILOCK_EXCL);
i_mmap_unlock_read(mapping);
if (error)
return error;
/* now unshare, allocate and add immutable flag */
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
