| Message ID | PAXP193MB2089D68F6B6E11464FB202FFA7F19@PAXP193MB2089.EURP193.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Check the return value of unpin_exten_cache. Cleanup style. | expand |
On 2023/1/1 02:47, Siddhartha Menon wrote: > Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com> > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8bcad9940154..cb95d47e4d02 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3331,7 +3331,7 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) > ordered_extent->disk_num_bytes); > } > } > - unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > + ret = unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > ordered_extent->num_bytes, trans->transid); Unfortunately this makes no difference, and in fact it's making the code much worse. That function unpin_extent_cache() won't return anything other than 0. Furthermore, by this we completely overwrite @ret, which is initially for insert_ordered_extent_file_extent() or btrfs_mark_extent_written(). This makes us to completely skip the error handling for write back. So NOACK. Thanks, Qu > if (ret < 0) { > btrfs_abort_transaction(trans, ret);
On Sun, Jan 01, 2023 at 08:00:10AM +0800, Qu Wenruo wrote: > > > On 2023/1/1 02:47, Siddhartha Menon wrote: > > Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com> > > --- > > fs/btrfs/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > > index 8bcad9940154..cb95d47e4d02 100644 > > --- a/fs/btrfs/inode.c > > +++ b/fs/btrfs/inode.c > > @@ -3331,7 +3331,7 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) > > ordered_extent->disk_num_bytes); > > } > > } > > - unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > > + ret = unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > > ordered_extent->num_bytes, trans->transid); > > Unfortunately this makes no difference, and in fact it's making the code > much worse. The return value should be there but not all errors are handled. > That function unpin_extent_cache() won't return anything other than 0. The errors after lookup_extent_mapping should be handled and not just WARN_ON.
On Sat, Dec 31, 2022 at 06:47:48PM +0000, Siddhartha Menon wrote: > Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com> > --- > fs/btrfs/inode.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 8bcad9940154..cb95d47e4d02 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -3331,7 +3331,7 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) > ordered_extent->disk_num_bytes); > } > } > - unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > + ret = unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, > ordered_extent->num_bytes, trans->transid); This is right but the errors need to be propagated upwards in the call chain and the call graph starting from that function must handle that properly, which is not true due to unhandled em = lookup_extent_mapping() error. Also please don't forget to write changelogs, this is not a trivial change where the subject would be sufficient.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 8bcad9940154..cb95d47e4d02 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -3331,7 +3331,7 @@ int btrfs_finish_ordered_io(struct btrfs_ordered_extent *ordered_extent) ordered_extent->disk_num_bytes); } } - unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, + ret = unpin_extent_cache(&inode->extent_tree, ordered_extent->file_offset, ordered_extent->num_bytes, trans->transid); if (ret < 0) { btrfs_abort_transaction(trans, ret);
Signed-off-by: Siddhartha Menon <siddharthamenon@outlook.com> --- fs/btrfs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)