On Mon, Jul 27, 2015 at 05:22:28PM +0800, Qu Wenruo wrote: > > > Filipe Manana wrote on 2015/07/27 09:26 +0100: > >On Mon, Jul 27, 2015 at 7:53 AM, Qu Wenruo <quwen...@cn.fujitsu.com> wrote: > >>Hi Filipe, > > > >Hi Qu, > > > >> > >>Sorry for the late reply after it is already merged, > >>but I'm a little concerned about the extra loop to find the first inc > >>delayed ref. > >> > >>It may take some extra time when there are a lot of delayed refs. > >> > >>What about allowing deleting the extent item at dec delayed ref time and > >>then add it back for later inc delayed refs? > > > >So, the reason I did it this way is simplicity - it's pretty much what > >the pre 4.2-rc1 [1] code did, but slightly more efficient because it > >iterates a linked list rather than a red black tree using rb_next(). > >So it seems somewhat odd that you're worrying about this after a > >functional fix when we pretty much always had this behavior in > >place... > > > >Given the complexity of what you propose, I would prefer if we have a > >benchmark that indeed shows this is a performance critical area (I > >don't think this list can get that huge, but I might be wrong). > I agree with you that this may not be a performance hotspot. > > And in fact, before the qgroup patchset, we tested the list implement of > delayed ref for performance regression, and the difference is so little that > can be easily ignored. > So I think even we skip the loop, the performance will only change little. > > One of the main purpose to change the delayed ref to list, is to make it a > little more straightforward, without the hard to understand/review rb tree > compare. > So I hope there is another more simple and straightforward one. > > But as you mentioned, your fix is already simple enough, and after a short > code search, my idea to fix may not be as simple as I thought. > > So sorry for the disturb, and thanks for your fix again.
Thanks for giving this some extra review. For now I'm happy with the tradeoff, but if it shows up in profiles we can look harder. -chris -- 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
