| Message ID | 8f91eea944203695995bd69512d7e0e37a39bd64.1608215738.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A variety of lock contention fixes | expand |
On 17.12.20 г. 16:36 ч., Josef Bacik wrote: > This was added a very long time ago to work around issues with delayed > refs and with qgroups. Both of these issues have since been properly > fixed, so all this does is cause a lot of lock contention with anybody > else who is running delayed refs. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Codewise it's ok, however I would have liked a better reference to the 2 problems being fixed, I'd assume same applies to David. SO it seems the first delayed refs run was added due to : 361048f586f5 ("Btrfs: fix full backref problem when inserting shared block reference") and the 2nd one by d67263354541 ("btrfs: qgroup: Make snapshot accounting work with new extent-oriented qgroup.") However there is no indication what code superseded the need for those 2 commits. > --- > fs/btrfs/transaction.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index 4776e055f7f9..6e3abe9b74c0 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -1686,12 +1686,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > goto fail; > } > > - ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); > - if (ret) { > - btrfs_abort_transaction(trans, ret); > - goto fail; > - } > - > /* > * Do special qgroup accounting for snapshot, as we do some qgroup > * snapshot hack to do fast snapshot. > @@ -1739,12 +1733,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, > } > } > > - ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); > - if (ret) { > - btrfs_abort_transaction(trans, ret); > - goto fail; > - } > - > fail: > pending->error = ret; > dir_item_existed: >
On 12/18/20 4:26 AM, Nikolay Borisov wrote: > > > On 17.12.20 г. 16:36 ч., Josef Bacik wrote: >> This was added a very long time ago to work around issues with delayed >> refs and with qgroups. Both of these issues have since been properly >> fixed, so all this does is cause a lot of lock contention with anybody >> else who is running delayed refs. >> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Codewise it's ok, however I would have liked a better reference to the 2 > problems being fixed, I'd assume same applies to David. SO it seems the > first delayed refs run was added due to : > > 361048f586f5 ("Btrfs: fix full backref problem when inserting shared > block reference") and the 2nd one by d67263354541 ("btrfs: qgroup: Make > snapshot accounting work with new extent-oriented qgroup.") > > > However there is no indication what code superseded the need for those 2 > commits. For 361048f586f5 it's because we now do #2 as described in that commit, which is to make sure we insert the actual extent entry first, and then deal with all subsequent extent reference modifications. The delayed refs code was quite fragile before, but we've gotten it into a good spot now so we no longer have this class of issues. And you bring up a good point for the second one, now that I look at the code. It appeared to me at first glance that the qgroup stuff had been reworked to no longer require the delayed refs flush, but it still does require it, so I'll rework this to move it into the appropriate helper and I'll update this commit message. Thanks, Josef
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 4776e055f7f9..6e3abe9b74c0 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -1686,12 +1686,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, goto fail; } - ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); - if (ret) { - btrfs_abort_transaction(trans, ret); - goto fail; - } - /* * Do special qgroup accounting for snapshot, as we do some qgroup * snapshot hack to do fast snapshot. @@ -1739,12 +1733,6 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans, } } - ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); - if (ret) { - btrfs_abort_transaction(trans, ret); - goto fail; - } - fail: pending->error = ret; dir_item_existed:
This was added a very long time ago to work around issues with delayed refs and with qgroups. Both of these issues have since been properly fixed, so all this does is cause a lot of lock contention with anybody else who is running delayed refs. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/transaction.c | 12 ------------ 1 file changed, 12 deletions(-)