Re: [PATCH v2 3/5] fs, xfs: introduce FALLOC_FL_UNSEAL_BLOCK_MAP

[Dan Williams]

On Fri, Aug 4, 2017 at 1:04 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
> On Thu, Aug 03, 2017 at 07:28:23PM -0700, Dan Williams wrote:
>> Provide an explicit fallocate operation type for clearing the
>> S_IOMAP_IMMUTABLE flag. Like the enable case it requires CAP_IMMUTABLE
>> and it can only be performed while no process has the file mapped.
>>
>> Cc: Jan Kara <jack@xxxxxxx>
>> Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
>> Cc: Christoph Hellwig <hch@xxxxxx>
>> Cc: Ross Zwisler <ross.zwisler@xxxxxxxxxxxxxxx>
>> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
>> Cc: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
>> Suggested-by: Dave Chinner <david@xxxxxxxxxxxxx>
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>>  fs/open.c                   |   17 +++++++++++------
>>  fs/xfs/xfs_bmap_util.c      |   42 ++++++++++++++++++++++++++++++++++++++++++
>>  fs/xfs/xfs_bmap_util.h      |    3 +++
>>  fs/xfs/xfs_file.c           |    4 +++-
>>  include/linux/falloc.h      |    3 ++-
>>  include/uapi/linux/falloc.h |    1 +
>>  6 files changed, 62 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/open.c b/fs/open.c
>> index e3aae59785ae..ccfd8d3becc8 100644
>> --- a/fs/open.c
>> +++ b/fs/open.c
>> @@ -274,13 +274,17 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>               return -EINVAL;
>>
>>       /*
>> -      * Seal block map operation should only be used exclusively, and
>> -      * with the IMMUTABLE capability.
>> +      * Seal/unseal block map operations should only be used
>> +      * exclusively, and with the IMMUTABLE capability.
>>        */
>> -     if (mode & FALLOC_FL_SEAL_BLOCK_MAP) {
>> +     if (mode & (FALLOC_FL_SEAL_BLOCK_MAP | FALLOC_FL_UNSEAL_BLOCK_MAP)) {
>>               if (!capable(CAP_LINUX_IMMUTABLE))
>>                       return -EPERM;
>> -             if (mode & ~FALLOC_FL_SEAL_BLOCK_MAP)
>> +             if (mode == (FALLOC_FL_SEAL_BLOCK_MAP
>> +                                     | FALLOC_FL_UNSEAL_BLOCK_MAP))
>> +                     return -EINVAL;
>> +             if (mode & ~(FALLOC_FL_SEAL_BLOCK_MAP
>> +                                     | FALLOC_FL_UNSEAL_BLOCK_MAP))
>>                       return -EINVAL;
>>       }
>>
>> @@ -303,9 +307,10 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>>               return -ETXTBSY;
>>
>>       /*
>> -      * We cannot allow any allocation changes on an iomap immutable file
>> +      * We cannot allow any allocation changes on an iomap immutable
>> +      * file, but we can allow clearing the immutable state.
>>        */
>> -     if (IS_IOMAP_IMMUTABLE(inode))
>> +     if (IS_IOMAP_IMMUTABLE(inode) && !(mode & FALLOC_FL_UNSEAL_BLOCK_MAP))
>>               return -ETXTBSY;
>>
>>       /*
>> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
>> index 46d8eb9e19fc..70ac2d33ab27 100644
>> --- a/fs/xfs/xfs_bmap_util.c
>> +++ b/fs/xfs/xfs_bmap_util.c
>> @@ -1494,6 +1494,48 @@ xfs_seal_file_space(
>>       return error;
>>  }
>>
>> +int
>> +xfs_unseal_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;
>> +
>> +     ASSERT(xfs_isilocked(ip, XFS_MMAPLOCK_EXCL));
>
> Same assert-on-the-iolock comment as the previous patch.

Ok.

>
>> +
>> +     if (offset)
>> +             return -EINVAL;
>> +
>> +     xfs_ilock(ip, XFS_ILOCK_EXCL);
>> +     /*
>> +      * It does not make sense to unseal less than the full range of
>> +      * the file.
>> +      */
>> +     error = -EINVAL;
>> +     if (len < i_size_read(inode))
>> +             goto out_unlock;
>
> Hmm, should we be picky and require len == i_size_read() here?

Yes, I think so, otherwise we may have raced someone who increased the
file size with unwritten extents.

>
>> +     /*
>> +      * Provide safety against one thread changing the policy of not
>> +      * requiring fsync/msync (for block allocations) behind another
>> +      * thread's back.
>> +      */
>> +     error = -EBUSY;
>> +     if (mapping_mapped(mapping))
>> +             goto out_unlock;
>> +
>> +     inode->i_flags &= ~S_IOMAP_IMMUTABLE;
>
> It occurred to me, should we jump out early from the seal/unseal
> operations if the flag state matches whatever the user is asking for?
> This is perhaps not necessary for unseal since we don't do a lot of
> work.
>

Yes, I think I had that semantic in v1, but lost in the cleanups. Will
bring it back.