Re: [PATCH v5 9/9] btrfs: implement RWF_ENCODED writes

[Omar Sandoval <osandov@osandov.com>]

On Mon, Aug 24, 2020 at 04:30:52PM -0400, Josef Bacik wrote:
> On 8/21/20 3:38 AM, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > The implementation resembles direct I/O: we have to flush any ordered
> > extents, invalidate the page cache, and do the io tree/delalloc/extent
> > map/ordered extent dance. From there, we can reuse the compression code
> > with a minor modification to distinguish the write from writeback. This
> > also creates inline extents when possible.
> > 
> > Now that read and write are implemented, this also sets the
> > FMODE_ENCODED_IO flag in btrfs_file_open().
> > 
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> >   fs/btrfs/compression.c  |   7 +-
> >   fs/btrfs/compression.h  |   6 +-
> >   fs/btrfs/ctree.h        |   2 +
> >   fs/btrfs/file.c         |  40 +++++--
> >   fs/btrfs/inode.c        | 246 +++++++++++++++++++++++++++++++++++++++-
> >   fs/btrfs/ordered-data.c |  12 +-
> >   fs/btrfs/ordered-data.h |   2 +
> >   7 files changed, 298 insertions(+), 17 deletions(-)
> > 
> 
> <snip>
> 
> > +
> > +	ret = btrfs_alloc_data_chunk_ondemand(BTRFS_I(inode), disk_num_bytes);
> > +	if (ret)
> > +		goto out_unlock;
> > +	ret = btrfs_qgroup_reserve_data(BTRFS_I(inode), &data_reserved, start,
> > +					num_bytes);
> > +	if (ret)
> > +		goto out_free_data_space;
> > +	ret = btrfs_delalloc_reserve_metadata(BTRFS_I(inode), num_bytes,
> > +					      disk_num_bytes);
> > +	if (ret)
> > +		goto out_qgroup_free_data;
> 
> This can just be btrfs_delalloc_reserve_space() and that way the error
> handling is much cleaner.
> 
> <snip>
> > +
> > +out_free_reserved:
> > +	btrfs_dec_block_group_reservations(fs_info, ins.objectid);
> > +	btrfs_free_reserved_extent(fs_info, ins.objectid, ins.offset, 1);
> > +out_delalloc_release:
> > +	btrfs_delalloc_release_extents(BTRFS_I(inode), num_bytes);
> > +	btrfs_delalloc_release_metadata(BTRFS_I(inode), disk_num_bytes,
> > +					ret < 0);
> 
> Likewise this can all just be btrfs_free_reserved_data_space().  Thanks,
> 
> Josef

btrfs_delalloc_reserve_space() and btrfs_free_reserved_data_space()
assume that num_bytes == disk_num_bytes, which isn't true for
RWF_ENCODED.

I figured it'd be cleaner to open-code this special case in the one
place that it's needed, but I could also add explicit num_bytes and
disk_num_bytes arguments to btrfs_delalloc_reserve_space() and
btrfs_free_reserved_data_space(). They'd just be equal everywhere except
for here.

If you're fine with keeping it this way, I'll add a comment explaining
why we can't use the higher-level helpers.