| Message ID | 70260eb8a1df6ad3b32ff4be62c9799fcc12ebc3.1677026757.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: dio partial write corruption fix | expand |
On Wed, Feb 22, 2023 at 1:04 AM Boris Burkov <boris@bur.io> wrote: > > Currently, btrfs_add_ordered_extent allocates a new ordered extent, adds > it to the rb_tree, but doesn't return a referenced pointer to the > caller. There are cases where it is useful for the creator of a new > ordered_extent to hang on to such a pointer, so add a new function > btrfs_alloc_ordered_extent which is the same as > btrfs_add_ordered_extent, except it takes an additional reference count > and returns a pointer to the ordered_extent. Implement > btrfs_add_ordered_extent as btrfs_alloc_ordered_extent followed by > dropping the new reference and handling the IS_ERR case. > > Signed-off-by: Boris Burkov <boris@bur.io> > --- > fs/btrfs/ordered-data.c | 45 ++++++++++++++++++++++++++++++++--------- > fs/btrfs/ordered-data.h | 7 ++++++- > 2 files changed, 42 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c > index 6c24b69e2d0a..35c082ef163e 100644 > --- a/fs/btrfs/ordered-data.c > +++ b/fs/btrfs/ordered-data.c > @@ -160,14 +160,16 @@ static inline struct rb_node *tree_search(struct btrfs_ordered_inode_tree *tree, > * @compress_type: Compression algorithm used for data. > * > * Most of these parameters correspond to &struct btrfs_file_extent_item. The > - * tree is given a single reference on the ordered extent that was inserted. > + * tree is given a single reference on the ordered extent that was inserted, and > + * the returned pointer is given a second reference. > * > - * Return: 0 or -ENOMEM. > + * Return: the new ordered_extent or ERR_PTR(-ENOMEM). Can we be consistent with the rest of the comments, and mention "ordered extent" instead of "ordered_extent"? After all, the latter is not a type name (which would be btrfs_ordered_extent). > */ > -int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > - u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, > - u64 disk_num_bytes, u64 offset, unsigned flags, > - int compress_type) > +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent( > + struct btrfs_inode *inode, u64 file_offset, > + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, > + u64 disk_num_bytes, u64 offset, unsigned long flags, > + int compress_type) > { > struct btrfs_root *root = inode->root; > struct btrfs_fs_info *fs_info = root->fs_info; > @@ -181,7 +183,7 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > /* For nocow write, we can release the qgroup rsv right now */ > ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes); > if (ret < 0) > - return ret; > + return ERR_PTR(ret); > ret = 0; > } else { > /* > @@ -190,11 +192,11 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > */ > ret = btrfs_qgroup_release_data(inode, file_offset, num_bytes); > if (ret < 0) > - return ret; > + return ERR_PTR(ret); > } > entry = kmem_cache_zalloc(btrfs_ordered_extent_cache, GFP_NOFS); > if (!entry) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > entry->file_offset = file_offset; > entry->num_bytes = num_bytes; > @@ -256,6 +258,31 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > btrfs_mod_outstanding_extents(inode, 1); > spin_unlock(&inode->lock); > > + /* one ref for the returned entry to match semantics of lookup */ > + refcount_inc(&entry->refs); > + return entry; > +} > + > + Double newline, use a single one. Can we also get a better subject than just "btrfs: btrfs_alloc_ordered_extent"? Perhaps something like: "btrfs: add function to create and return an ordered extent". Those are just minor things, David can fix them up when he picks the patch if he wants to, so: Reviewed-by: Filipe Manana <fdmanana@suse.com> > +/* > + * Add a new btrfs_ordered_extent for the range, but drop the reference > + * instead of returning it to the caller. > + */ > +int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, > + u64 disk_num_bytes, u64 offset, unsigned long flags, > + int compress_type) > +{ > + struct btrfs_ordered_extent *ordered; > + > + ordered = btrfs_alloc_ordered_extent(inode, file_offset, num_bytes, > + ram_bytes, disk_bytenr, > + disk_num_bytes, offset, flags, > + compress_type); > + > + if (IS_ERR(ordered)) > + return PTR_ERR(ordered); > + btrfs_put_ordered_extent(ordered); > return 0; > } > > diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h > index eb40cb39f842..18007f9c00ad 100644 > --- a/fs/btrfs/ordered-data.h > +++ b/fs/btrfs/ordered-data.h > @@ -178,9 +178,14 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode, > bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode, > struct btrfs_ordered_extent **cached, > u64 file_offset, u64 io_size); > +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent( > + struct btrfs_inode *inode, u64 file_offset, > + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, > + u64 disk_num_bytes, u64 offset, unsigned long flags, > + int compress_type); > int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, > u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, > - u64 disk_num_bytes, u64 offset, unsigned flags, > + u64 disk_num_bytes, u64 offset, unsigned long flags, > int compress_type); > void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry, > struct btrfs_ordered_sum *sum); > -- > 2.38.1 >
On Tue, Feb 21, 2023 at 04:49:59PM -0800, Boris Burkov wrote: > Currently, btrfs_add_ordered_extent allocates a new ordered extent, adds > it to the rb_tree, but doesn't return a referenced pointer to the > caller. There are cases where it is useful for the creator of a new > ordered_extent to hang on to such a pointer, so add a new function > btrfs_alloc_ordered_extent which is the same as > btrfs_add_ordered_extent, except it takes an additional reference count > and returns a pointer to the ordered_extent. Implement > btrfs_add_ordered_extent as btrfs_alloc_ordered_extent followed by > dropping the new reference and handling the IS_ERR case. This also changes the existing flags argument to btrfs_add_ordered_extent to an unsigned long, which needs to be explained or even better split out into a separate patch. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c index 6c24b69e2d0a..35c082ef163e 100644 --- a/fs/btrfs/ordered-data.c +++ b/fs/btrfs/ordered-data.c @@ -160,14 +160,16 @@ static inline struct rb_node *tree_search(struct btrfs_ordered_inode_tree *tree, * @compress_type: Compression algorithm used for data. * * Most of these parameters correspond to &struct btrfs_file_extent_item. The - * tree is given a single reference on the ordered extent that was inserted. + * tree is given a single reference on the ordered extent that was inserted, and + * the returned pointer is given a second reference. * - * Return: 0 or -ENOMEM. + * Return: the new ordered_extent or ERR_PTR(-ENOMEM). */ -int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, - u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, - u64 disk_num_bytes, u64 offset, unsigned flags, - int compress_type) +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent( + struct btrfs_inode *inode, u64 file_offset, + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, + u64 disk_num_bytes, u64 offset, unsigned long flags, + int compress_type) { struct btrfs_root *root = inode->root; struct btrfs_fs_info *fs_info = root->fs_info; @@ -181,7 +183,7 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, /* For nocow write, we can release the qgroup rsv right now */ ret = btrfs_qgroup_free_data(inode, NULL, file_offset, num_bytes); if (ret < 0) - return ret; + return ERR_PTR(ret); ret = 0; } else { /* @@ -190,11 +192,11 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, */ ret = btrfs_qgroup_release_data(inode, file_offset, num_bytes); if (ret < 0) - return ret; + return ERR_PTR(ret); } entry = kmem_cache_zalloc(btrfs_ordered_extent_cache, GFP_NOFS); if (!entry) - return -ENOMEM; + return ERR_PTR(-ENOMEM); entry->file_offset = file_offset; entry->num_bytes = num_bytes; @@ -256,6 +258,31 @@ int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, btrfs_mod_outstanding_extents(inode, 1); spin_unlock(&inode->lock); + /* one ref for the returned entry to match semantics of lookup */ + refcount_inc(&entry->refs); + return entry; +} + + +/* + * Add a new btrfs_ordered_extent for the range, but drop the reference + * instead of returning it to the caller. + */ +int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, + u64 disk_num_bytes, u64 offset, unsigned long flags, + int compress_type) +{ + struct btrfs_ordered_extent *ordered; + + ordered = btrfs_alloc_ordered_extent(inode, file_offset, num_bytes, + ram_bytes, disk_bytenr, + disk_num_bytes, offset, flags, + compress_type); + + if (IS_ERR(ordered)) + return PTR_ERR(ordered); + btrfs_put_ordered_extent(ordered); return 0; } diff --git a/fs/btrfs/ordered-data.h b/fs/btrfs/ordered-data.h index eb40cb39f842..18007f9c00ad 100644 --- a/fs/btrfs/ordered-data.h +++ b/fs/btrfs/ordered-data.h @@ -178,9 +178,14 @@ void btrfs_mark_ordered_io_finished(struct btrfs_inode *inode, bool btrfs_dec_test_ordered_pending(struct btrfs_inode *inode, struct btrfs_ordered_extent **cached, u64 file_offset, u64 io_size); +struct btrfs_ordered_extent *btrfs_alloc_ordered_extent( + struct btrfs_inode *inode, u64 file_offset, + u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, + u64 disk_num_bytes, u64 offset, unsigned long flags, + int compress_type); int btrfs_add_ordered_extent(struct btrfs_inode *inode, u64 file_offset, u64 num_bytes, u64 ram_bytes, u64 disk_bytenr, - u64 disk_num_bytes, u64 offset, unsigned flags, + u64 disk_num_bytes, u64 offset, unsigned long flags, int compress_type); void btrfs_add_ordered_sum(struct btrfs_ordered_extent *entry, struct btrfs_ordered_sum *sum);
Currently, btrfs_add_ordered_extent allocates a new ordered extent, adds it to the rb_tree, but doesn't return a referenced pointer to the caller. There are cases where it is useful for the creator of a new ordered_extent to hang on to such a pointer, so add a new function btrfs_alloc_ordered_extent which is the same as btrfs_add_ordered_extent, except it takes an additional reference count and returns a pointer to the ordered_extent. Implement btrfs_add_ordered_extent as btrfs_alloc_ordered_extent followed by dropping the new reference and handling the IS_ERR case. Signed-off-by: Boris Burkov <boris@bur.io> --- fs/btrfs/ordered-data.c | 45 ++++++++++++++++++++++++++++++++--------- fs/btrfs/ordered-data.h | 7 ++++++- 2 files changed, 42 insertions(+), 10 deletions(-)