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.
