| Message ID | 913ab94d58070b19dee7aa6760a111c31be473a1.1718816796.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Error handling fixes | expand |
On Wed, Jun 19, 2024 at 07:09:20PM +0200, David Sterba wrote: > Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to > update the qgroup status is probably not necessary, this would turn the > filesystem to read-only. For the same reason aborting the transaction is > also not a good option. > > The state is left inconsistent and can be fixed by rescan, printing a > warning should be sufficient. Return code reflects the status of > adding/deleting the relation and if the transaction was ended properly. A few thoughts/questions about this: 1. Why do we even need to btrfs_run_qgroups() here? Won't we btrfs_run_qgroups in the transaction that actually commits the qgroup relation items? I'm guessing some qgroup lookup sees the not-committed items in a way users care about? Are we expecting such high-scale `qgroup assign` that we can't just commit this txn and make it simpler? 2. Is this really failing in cases where adding the relation items succeeds and then it all gets committed successfully? i.e., do you have a reproducer for this case? 3. If this is a realistic scenario, I'm worried about the squotas case, which doesn't have any rescan capability to fallback on. However, I don't see why my (1) above doesn't just save us anyway. If we commit the relation item, then we also commit the status/info items. And the txn commit run_qgroups is not allowed to fail. 4. Let's say that 1. is not strictly essential, and the txn commit is going to fix us. In that case, is it really accurate to say we are inconsistent any more than just during a transaction before we run qgroups? I suppose compared to the case where this succeeded, we are. I just have a weird feeling we are stretching the meaning of inconsistent. Though not in an egregious way or anything, as we are reporting a non-fatal error? Sorry for the slightly rambling review.. Thanks, Boris > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/ioctl.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 31c4aea8b93a..f893a6b711c6 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) > err = btrfs_run_qgroups(trans); > mutex_unlock(&fs_info->qgroup_ioctl_lock); > if (err < 0) > - btrfs_handle_fs_error(fs_info, err, > - "failed to update qgroup status and info"); > + btrfs_warn(fs_info, > + "qgroup status update failed after %s relation, marked as inconsistent", > + sa->assign ? "adding" : "deleting"); > err = btrfs_end_transaction(trans); > if (err && !ret) > ret = err; > -- > 2.45.0 >
在 2024/6/21 06:58, Boris Burkov 写道: > On Wed, Jun 19, 2024 at 07:09:20PM +0200, David Sterba wrote: >> Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to >> update the qgroup status is probably not necessary, this would turn the >> filesystem to read-only. For the same reason aborting the transaction is >> also not a good option. >> >> The state is left inconsistent and can be fixed by rescan, printing a >> warning should be sufficient. Return code reflects the status of >> adding/deleting the relation and if the transaction was ended properly. > > A few thoughts/questions about this: > > 1. Why do we even need to btrfs_run_qgroups() here? Won't we btrfs_run_qgroups > in the transaction that actually commits the qgroup relation items? I'm > guessing some qgroup lookup sees the not-committed items in a way users > care about? Are we expecting such high-scale `qgroup assign` that we > can't just commit this txn and make it simpler? I agree with this. I'm originally thinking some weird situation that one have called btrfs_run_qgroups(), then we do relation update without btrfs_run_qgroups(), in this case it may cause problems. But since btrfs_run_qgroups() is really only called in committing transaction and this is the only other location, I do not think it's going to cause much problem. So overall we should just remove the btrfs_run_qgroups() call and no need to handle the error (even no need to commit tranasaction). Thanks, Qu > > 2. Is this really failing in cases where adding the relation items > succeeds and then it all gets committed successfully? i.e., do you have > a reproducer for this case? > > 3. If this is a realistic scenario, I'm worried about the squotas case, > which doesn't have any rescan capability to fallback on. However, I > don't see why my (1) above doesn't just save us anyway. If we commit the > relation item, then we also commit the status/info items. And the txn > commit run_qgroups is not allowed to fail. > > 4. Let's say that 1. is not strictly essential, and the txn commit is > going to fix us. In that case, is it really accurate to say we are > inconsistent any more than just during a transaction before we run > qgroups? I suppose compared to the case where this succeeded, we are. I > just have a weird feeling we are stretching the meaning of inconsistent. > Though not in an egregious way or anything, as we are reporting a > non-fatal error? > > Sorry for the slightly rambling review.. > > Thanks, > Boris > >> >> Signed-off-by: David Sterba <dsterba@suse.com> >> --- >> fs/btrfs/ioctl.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c >> index 31c4aea8b93a..f893a6b711c6 100644 >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) >> err = btrfs_run_qgroups(trans); >> mutex_unlock(&fs_info->qgroup_ioctl_lock); >> if (err < 0) >> - btrfs_handle_fs_error(fs_info, err, >> - "failed to update qgroup status and info"); >> + btrfs_warn(fs_info, >> + "qgroup status update failed after %s relation, marked as inconsistent", >> + sa->assign ? "adding" : "deleting"); >> err = btrfs_end_transaction(trans); >> if (err && !ret) >> ret = err; >> -- >> 2.45.0 >> >
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 31c4aea8b93a..f893a6b711c6 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -3877,8 +3877,9 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) err = btrfs_run_qgroups(trans); mutex_unlock(&fs_info->qgroup_ioctl_lock); if (err < 0) - btrfs_handle_fs_error(fs_info, err, - "failed to update qgroup status and info"); + btrfs_warn(fs_info, + "qgroup status update failed after %s relation, marked as inconsistent", + sa->assign ? "adding" : "deleting"); err = btrfs_end_transaction(trans); if (err && !ret) ret = err;
Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to update the qgroup status is probably not necessary, this would turn the filesystem to read-only. For the same reason aborting the transaction is also not a good option. The state is left inconsistent and can be fixed by rescan, printing a warning should be sufficient. Return code reflects the status of adding/deleting the relation and if the transaction was ended properly. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/ioctl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)