On 2018年03月08日 00:02, David Sterba wrote:
> On Thu, Feb 22, 2018 at 10:05:36AM +0800, Qu Wenruo wrote:
>> On 2018年02月22日 09:50, Jeff Mahoney wrote:
>>> On 2/21/18 8:36 PM, Qu Wenruo wrote:
>>>> On 2018年02月22日 04:19, je...@suse.com wrote:
>>>>> From: Jeff Mahoney <je...@suse.com>
>>>>> 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.
>>> 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.


> 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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to