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. 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
