On Fri, Aug 20, 2021 at 05:13:34PM +0800, Qu Wenruo wrote:
>
>
> On 2021/8/20 下午4:51, Nikolay Borisov wrote:
> >
> >
> > On 18.08.21 г. 0:06, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@xxxxxx>
> > >
> > > Currently, an inline extent is always created after i_size is extended
> > > from btrfs_dirty_pages(). However, for encoded writes, we only want to
> > > update i_size after we successfully created the inline extent.
>
> To me, the idea of write first then update isize is just going to cause
> tons of inline extent related prblems.
>
> The current example is falloc, which only update the isize after the
> falloc finishes.
>
> This behavior has already bothered me quite a lot, as it can easily
> create mixed inline and regular extents.
Do you have an example of how this would happen? I have the inode and
extent bits locked during an encoded write, and I see that fallocate
does the same.
> Can't we remember the old isize (with proper locking), enlarge isize
> (with holes filled), do the write.
>
> If something wrong happened, we truncate the isize back to its old isize.
>
> > > Add an
> > > update_i_size parameter to cow_file_range_inline() and
> > > insert_inline_extent() and pass in the size of the extent rather than
> > > determining it from i_size. Since the start parameter is always passed
> > > as 0, get rid of it and simplify the logic in these two functions. While
> > > we're here, let's document the requirements for creating an inline
> > > extent.
> > >
> > > Reviewed-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> > > Signed-off-by: Omar Sandoval <osandov@xxxxxx>
> > > ---
> > > fs/btrfs/inode.c | 100 +++++++++++++++++++++++------------------------
> > > 1 file changed, 48 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> > > index 708d8ab098bc..0b5ff14aa7fd 100644
> > > --- a/fs/btrfs/inode.c
> > > +++ b/fs/btrfs/inode.c
> > > @@ -236,9 +236,10 @@ static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
> > > static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > struct btrfs_path *path, bool extent_inserted,
> > > struct btrfs_root *root, struct inode *inode,
> > > - u64 start, size_t size, size_t compressed_size,
> > > + size_t size, size_t compressed_size,
> > > int compress_type,
> > > - struct page **compressed_pages)
> > > + struct page **compressed_pages,
> > > + bool update_i_size)
> > > {
> > > struct extent_buffer *leaf;
> > > struct page *page = NULL;
> > > @@ -247,7 +248,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > struct btrfs_file_extent_item *ei;
> > > int ret;
> > > size_t cur_size = size;
> > > - unsigned long offset;
> > > + u64 i_size;
> > >
> > > ASSERT((compressed_size > 0 && compressed_pages) ||
> > > (compressed_size == 0 && !compressed_pages));
> > > @@ -260,7 +261,7 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > size_t datasize;
> > >
> > > key.objectid = btrfs_ino(BTRFS_I(inode));
> > > - key.offset = start;
> > > + key.offset = 0;
> > > key.type = BTRFS_EXTENT_DATA_KEY;
> > >
> > > datasize = btrfs_file_extent_calc_inline_size(cur_size);
> > > @@ -297,12 +298,10 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > btrfs_set_file_extent_compression(leaf, ei,
> > > compress_type);
> > > } else {
> > > - page = find_get_page(inode->i_mapping,
> > > - start >> PAGE_SHIFT);
> > > + page = find_get_page(inode->i_mapping, 0);
> > > btrfs_set_file_extent_compression(leaf, ei, 0);
> > > kaddr = kmap_atomic(page);
> > > - offset = offset_in_page(start);
> > > - write_extent_buffer(leaf, kaddr + offset, ptr, size);
> > > + write_extent_buffer(leaf, kaddr, ptr, size);
> > > kunmap_atomic(kaddr);
> > > put_page(page);
> > > }
> > > @@ -313,8 +312,8 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > * We align size to sectorsize for inline extents just for simplicity
> > > * sake.
> > > */
> > > - size = ALIGN(size, root->fs_info->sectorsize);
> > > - ret = btrfs_inode_set_file_extent_range(BTRFS_I(inode), start, size);
> > > + ret = btrfs_inode_set_file_extent_range(BTRFS_I(inode), 0,
> > > + ALIGN(size, root->fs_info->sectorsize));
> > > if (ret)
> > > goto fail;
> > >
> > > @@ -327,7 +326,13 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > * before we unlock the pages. Otherwise we
> > > * could end up racing with unlink.
> > > */
> > > - BTRFS_I(inode)->disk_i_size = inode->i_size;
> > > + i_size = i_size_read(inode);
> > > + if (update_i_size && size > i_size) {
> > > + i_size_write(inode, size);
> > > + i_size = size;
> > > + }
> > > + BTRFS_I(inode)->disk_i_size = i_size;
> > > +
> > > fail:
> > > return ret;
> > > }
> > > @@ -338,35 +343,31 @@ static int insert_inline_extent(struct btrfs_trans_handle *trans,
> > > * does the checks required to make sure the data is small enough
> > > * to fit as an inline extent.
> > > */
> > > -static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 start,
> > > - u64 end, size_t compressed_size,
> > > +static noinline int cow_file_range_inline(struct btrfs_inode *inode, u64 size,
> > > + size_t compressed_size,
> > > int compress_type,
> > > - struct page **compressed_pages)
> > > + struct page **compressed_pages,
> > > + bool update_i_size)
> > > {
> > > struct btrfs_drop_extents_args drop_args = { 0 };
> > > struct btrfs_root *root = inode->root;
> > > struct btrfs_fs_info *fs_info = root->fs_info;
> > > struct btrfs_trans_handle *trans;
> > > - u64 isize = i_size_read(&inode->vfs_inode);
> > > - u64 actual_end = min(end + 1, isize);
> > > - u64 inline_len = actual_end - start;
> > > - u64 aligned_end = ALIGN(end, fs_info->sectorsize);
> > > - u64 data_len = inline_len;
> > > + u64 data_len = compressed_size ? compressed_size : size;
> > > int ret;
> > > struct btrfs_path *path;
> > >
> > > - if (compressed_size)
> > > - data_len = compressed_size;
> > > -
> > > - if (start > 0 ||
> > > - actual_end > fs_info->sectorsize ||
> > > + /*
> > > + * We can create an inline extent if it ends at or beyond the current
> > > + * i_size, is no larger than a sector (decompressed), and the (possibly
> > > + * compressed) data fits in a leaf and the configured maximum inline
> > > + * size.
> > > + */
> >
> > Urgh, just some days ago Qu was talking about how awkward it is to have
> > mixed extents in a file. And now, AFAIU, you are making them more likely
> > since now they can be created not just at the beginning of the file but
> > also after i_size write. While this won't be a problem in and of itself
> > it goes just the opposite way of us trying to shrink the possible cases
> > when we can have mixed extents.
>
> Tree-checker should reject such inline extent at non-zero offset.
This change does not allow creating inline extents at a non-zero offset.
> > Qu what is your take on that?
>
> My question is, why encoded write needs to bother the inline extents at all?
>
> My intuition of such encoded write is, it should not create inline
> extents at all.
>
> Or is there any special use-case involved for encoded write?
We create compressed inline extents with normal writes. We should be
able to send and receive them without converting them into regular
extents.
