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