Re: [PATCH v2 5/5] btrfs: enable swap file support

[Filipe David Manana]

On Fri, Nov 21, 2014 at 6:00 PM, David Sterba <dsterba@xxxxxxx> wrote:
> On Fri, Nov 21, 2014 at 02:08:31AM -0800, Omar Sandoval wrote:
>> Implement the swap file a_ops on btrfs. Activation simply checks for a usable
>> swap file: it must be fully allocated (no holes), support direct I/O (so no
>> compressed or inline extents) and should be nocow (I'm not sure about that last
>> one).
>>
>> Signed-off-by: Omar Sandoval <osandov@xxxxxxxxxxx>
>> ---
>>  fs/btrfs/inode.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 71 insertions(+)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index d23362f..b8fd36b 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -9442,6 +9442,75 @@ out_inode:
>>
>>  }
>>
>> +static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file,
>> +                            sector_t *span)
>> +{
>> +     struct inode *inode = file_inode(file);
>> +     struct btrfs_inode *ip = BTRFS_I(inode);
>
> 'ip' looks strange in context of a filesystem, please pick a different
> name (eg. 'inode' or whatever).
>
>> +     int ret = 0;
>> +     u64 isize = inode->i_size;
>> +     struct extent_state *cached_state = NULL;
>> +     struct extent_map *em;
>> +     u64 start, len;
>> +
>> +     if (ip->flags & BTRFS_INODE_COMPRESS) {
>> +             /* Can't do direct I/O on a compressed file. */
>> +             pr_err("BTRFS: swapfile is compressed");
>
> Please use the btrfs_err macros everywhere.
>
>> +             return -EINVAL;
>> +     }
>> +     if (!(ip->flags & BTRFS_INODE_NODATACOW)) {
>> +             /* The swap file can't be copy-on-write. */
>> +             pr_err("BTRFS: swapfile is copy-on-write");
>> +             return -EINVAL;
>> +     }
>> +
>> +     lock_extent_bits(&ip->io_tree, 0, isize - 1, 0, &cached_state);
>> +
>> +     /*
>> +      * All of the extents must be allocated and support direct I/O. Inline
>> +      * extents and compressed extents fall back to buffered I/O, so those
>> +      * are no good.
>> +      */
>> +     start = 0;
>> +     while (start < isize) {
>> +             len = isize - start;
>> +             em = btrfs_get_extent(inode, NULL, 0, start, len, 0);
>> +             if (IS_ERR(em)) {
>> +                     ret = PTR_ERR(em);
>> +                     goto out;
>> +             }
>> +
>> +             if (test_bit(EXTENT_FLAG_VACANCY, &em->flags) ||
>> +                 em->block_start == EXTENT_MAP_HOLE) {
>
> If the no-holes feature is enabled on the filesystem, there won't be any
> such extent representing the hole. You have to check that each two
> consecutive extents are adjacent.

If no-holes is enabled it means file extent items aren't used to
represent holes. But extent maps are still used to represent holes in
memory, and that's what the code is looking for and therefore it's
correct.

>
>> +                     pr_err("BTRFS: swapfile has holes");
>> +                     ret = -EINVAL;
>> +                     goto out;
>> +             }
>> +             if (em->block_start == EXTENT_MAP_INLINE) {
>> +                     pr_err("BTRFS: swapfile is inline");
>
> While the test is valid, this would mean that the file is smaller than
> the inline limit, which is now one page. I think the generic swap code
> would refuse such a small file anyway.
>
>> +                     ret = -EINVAL;
>> +                     goto out;
>> +             }
>> +             if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags)) {
>> +                     pr_err("BTRFS: swapfile is compresed");
>> +                     ret = -EINVAL;
>> +                     goto out;
>> +             }
>
> I think the preallocated extents should be refused as well. This means
> the filesystem has enough space to hold the data but it would still have
> to go through the allocation and could in turn stress the memory
> management code that triggered the swapping activity in the first place.
>
> Though it's probably still possible to reach such corner case even with
> fully allocated nodatacow file, this should be reviewed anyway.
>
>> +
>> +             start = extent_map_end(em);
>> +             free_extent_map(em);
>> +     }
>> +
>> +out:
>> +     unlock_extent_cached(&ip->io_tree, 0, isize - 1, &cached_state,
>> +                          GFP_NOFS);
>> +     return ret;
>> +}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/