| Message ID | 602641114c0a6c5ba07f78371a4d94fd1c442218.1606938211.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Cleanup error handling in relocation | expand |
On 2020/12/3 上午3:50, Josef Bacik wrote: > record_root_in_trans can currently fail, so handle this failure > properly. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Qu Wenruo <wqu@suse.com> But I guess it would be better to folding patch 17~26 into one big patch. Since each of them are really small. Thanks, Qu > --- > fs/btrfs/transaction.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 087d919de9fb..5393c0c4926c 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1568,8 +1568,9 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > dentry = pending->dentry; > parent_inode = pending->dir; > parent_root = BTRFS_I(parent_inode)->root; > - record_root_in_trans(trans, parent_root, 0); > - > + ret = record_root_in_trans(trans, parent_root, 0); > + if (ret) > + goto fail; > cur_time = current_time(parent_inode); > > /* > @@ -1605,7 +1606,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > goto fail; > } > > - record_root_in_trans(trans, root, 0); > + ret = record_root_in_trans(trans, root, 0); > + if (ret) { > + btrfs_abort_transaction(trans, ret); > + goto fail; > + } > btrfs_set_root_last_snapshot(&root->root_item, trans->transid); > memcpy(new_root_item, &root->root_item, sizeof(*new_root_item)); > btrfs_check_and_init_root_item(new_root_item); >
On 12/2/20 9:56 PM, Qu Wenruo wrote: > > > On 2020/12/3 上午3:50, Josef Bacik wrote: >> record_root_in_trans can currently fail, so handle this failure >> properly. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > But I guess it would be better to folding patch 17~26 into one big patch. > > Since each of them are really small. > I don't like to do that because it makes it easier for us to just gloss over the change rather than checking each site. You prove my point by noticing that I wasn't dropping the new_root ref in the error case for btrfs: handle btrfs_record_root_in_trans failure in create_subvol It would have been easy for you to gloss over that change if it were in a giant patch. I find it nice to have it in distinct patches so I'm forced to check the context of every patch I'm reviewing. Thanks, Josef
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 087d919de9fb..5393c0c4926c 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1568,8 +1568,9 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, dentry = pending->dentry; parent_inode = pending->dir; parent_root = BTRFS_I(parent_inode)->root; - record_root_in_trans(trans, parent_root, 0); - + ret = record_root_in_trans(trans, parent_root, 0); + if (ret) + goto fail; cur_time = current_time(parent_inode); /* @@ -1605,7 +1606,11 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, goto fail; } - record_root_in_trans(trans, root, 0); + ret = record_root_in_trans(trans, root, 0); + if (ret) { + btrfs_abort_transaction(trans, ret); + goto fail; + } btrfs_set_root_last_snapshot(&root->root_item, trans->transid); memcpy(new_root_item, &root->root_item, sizeof(*new_root_item)); btrfs_check_and_init_root_item(new_root_item);
record_root_in_trans can currently fail, so handle this failure properly. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/transaction.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)