On 2018年03月08日 00:02, David Sterba wrote:

On Thu, Feb 22, 2018 at 10:05:36AM +0800, Qu Wenruo wrote:

quoted

On 2018年02月22日 09:50, Jeff Mahoney wrote:

quoted

On 2/21/18 8:36 PM, Qu Wenruo wrote:

quoted

On 2018年02月22日 04:19, jeffm@suse.com wrote:

quoted

From: Jeff Mahoney <redacted>

There are several places where we call btrfs_qgroup_reserve_meta and
assume that a return value of 0 means that the reservation was successful.

Later, we use the original bytes value passed to that call to free
bytes during error handling or to pass the number of bytes reserved to
the caller.

This patch returns -ENODATA when we don't perform a reservation so that
callers can make the distinction.  This also lets call sites not
necessarily care whether qgroups are enabled.

IMHO if we don't need to reserve, returning 0 seems good enough.
Caller doesn't really need to care if it has reserved some bytes.

Or is there any special case where we need to distinguish such case?

Anywhere where the reservation takes place prior to the transaction
starting, which is pretty much everywhere.  We wait until transaction
commit to flip the bit to turn on quotas, which means that if a
transaction commits that enables quotas lands in between the reservation
being take and any error handling that involves freeing the reservation,
we'll end up with an underflow.

So the same case as btrfs_qgroup_reserve_data().

In that case we could use ret > 0 to indicates the real bytes we
reserved, instead of -ENODATA which normally means error.

quoted

This is the first patch of a series I'm working on, but it can stand
alone.  The rest is the patch set I mentioned when we talked a few
months ago where the lifetimes of reservations are incorrect.  We can't
just drop all the reservations at the end of the transaction because 1)
the lifetime of some reservations can cross transactions and 2) because
especially in the start_transaction case, we do the reservation prior to
waiting to join the transaction.  So if the transaction we're waiting on
commits, our reservation goes away with it but we continue on as if we
still have it.

Right, the same problem I also addressed in patchset "[PATCH v2 00/10]
Use split qgroup rsv type".

In 6th patch, "[PATCH v2 06/10] btrfs: qgroup: Use
root->qgroup_meta_rsv_* to record qgroup meta reserved space" qgroup
meta reserve will only be increased if we succeeded in reserving
metadata, so later free won't underflow that number.

What should we do now when there are 2 different fixes? Applying Jeff's
patch on top of your qgroup-types patches causes some conflicts that do
not seem to be difficult, but the end result might not work as expected.

Jeff's patch is a more pinpoint solution to handle metadata reservation
error, while mine is a generic one which adds another layer to catch
possible underflow.

I think both could co-exist.

The only thing I'm not a fan is the return value of -ENODATA.
Despite that it should be fine.

Thanks,
Qu

The changes do not seem to be fundamentally conflicting, would it make
sense to merge both?
The patchset has been in for-next for a while but I
don't run qgroup specific tests besides what's in fstests. Also the
patchset fixes more problems so I think we need to merge it at some
point and now it's a good time about deciding whether it'd go to 4.17.

I did a pass through the patches, there are some minor things to fix but
a review from somebody with qgroup knowledge would be still desirable.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html