Re: [PATCH v2 5/5] xfs: toggle XFS_DIFLAG2_IOMAP_IMMUTABLE in response to fallocate

[Darrick J. Wong]

On Fri, Aug 04, 2017 at 01:47:32PM -0700, Dan Williams wrote:
> On Fri, Aug 4, 2017 at 1:14 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> > On Thu, Aug 03, 2017 at 07:28:35PM -0700, Dan Williams wrote:
> >> After validating the state of the file as not having holes, shared
> >> extents, or active mappings try to commit the
> >> XFS_DIFLAG2_IOMAP_IMMUTABLE flag to the on-disk inode metadata. If that
> >> succeeds then allow the S_IOMAP_IMMUTABLE to be set on the vfs inode.
> >>
> >> Cc: Jan Kara <jack@xxxxxxx>
> >> Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
> >> Cc: Christoph Hellwig <hch@xxxxxx>
> >> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
> >> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
> >> Suggested-by: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
> >> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> >> ---
> >>  fs/xfs/xfs_bmap_util.c |   32 ++++++++++++++++++++++++++++++++
> >>  1 file changed, 32 insertions(+)
> >>
> >> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> >> index 70ac2d33ab27..8464c25a2403 100644
> >> --- a/fs/xfs/xfs_bmap_util.c
> >> +++ b/fs/xfs/xfs_bmap_util.c
> >> @@ -1436,9 +1436,11 @@ xfs_seal_file_space(
> >>       xfs_off_t               offset,
> >>       xfs_off_t               len)
> >>  {
> >> +     struct xfs_mount        *mp = ip->i_mount;
> >>       struct inode            *inode = VFS_I(ip);
> >>       struct address_space    *mapping = inode->i_mapping;
> >>       int                     error;
> >> +     struct xfs_trans        *tp;
> >>
> >>       ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
> >>
> >> @@ -1454,6 +1456,10 @@ xfs_seal_file_space(
> >>       if (error)
> >>               return error;
> >>
> >> +     error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> >> +     if (error)
> >> +             return error;
> >> +
> >>       xfs_ilock(ip, XFS_ILOCK_EXCL);
> >>       /*
> >>        * Either the size changed after we performed allocation /
> >> @@ -1486,10 +1492,20 @@ xfs_seal_file_space(
> >>       if (error < 0)
> >>               goto out_unlock;
> >>
> >> +     xfs_trans_ijoin(tp, ip, 0);
> >
> > FWIW if you change that third parameter to XFS_ILOCK_EXCL then
> > xfs_trans_commit will do the xfs_iunlock(ip, XFS_ILOCK_EXCL) for you if
> > the commit succeeds...
> >
> >> +     ip->i_d.di_flags2 |= XFS_DIFLAG2_IOMAP_IMMUTABLE;
> >> +     xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> >> +     error = xfs_trans_commit(tp);
> >> +     tp = NULL; /* nothing to cancel */
> >> +     if (error)
> >> +             goto out_unlock;
> >> +
> >>       inode->i_flags |= S_IOMAP_IMMUTABLE;
> >
> > ...and then you can just return out here.
> 
> Do we not need to hold XFS_ILOCK_EXCL over ->i_flags changes, or is
> XFS_IOLOCK_EXCL enough?

Oh, heh, I missed a piece.  Set the flag before the transaction commit,
because if the commit fails the fs is shut down anyway. :)

--D

> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html