On Tue, Apr 12, 2016 at 12:41:35PM +1000, Dave Chinner wrote: > On Mon, Apr 11, 2016 at 06:16:26PM -0700, Darrick J. Wong wrote: > > [should have kicked this onto the lists...] > > > > This needs to be list_add_tail so that the rmap operations happen in order. > > > > > > Ah, no, it doesn't. The list_sort() before we process it destroys > > > any order we create here. > > > > I think list_sort() is supposed to maintain list order if the compare fn > > returns zero. From lib/list_sort.c: > > > > /* > > <snip> > > * The comparison function @cmp must return a negative value if @a > > * should sort before @b, and a positive value if @a should sort after > > * @b. If @a and @b are equivalent, and their original relative > > * ordering is to be preserved, @cmp must return 0. > > */ > > > > From my (admittedly flimsy) testing of list_sort() it looks like it does > > preserve the order of rmap_intents that have the same agno. I looked at > > the source code and I think that assertion should hold. > > If what I did is maintaining order, then it's simply by chance, not > intent. :/ :) I guess the scary part is what if list_sort ever fails to maintain order amongst the cmp() == 0 items, but I'll put in a generous HBD note before the list_add_tail and the list_sort. > > > > we do a collapse range of 2 blocks at offset 32, say, then we end up with > > > > unmap($pblk, $ino, 32, 16) and map($pblk-2, $ino, 30, 16) in the deferred list. > > > > We don't want the map to happen before the unmap because that results in two > > > > rmaps covering the same range of logical offset, which causes problems in > > > > generic/145. > > > > I forgot to mention this, but the ordering requirement only applies within a > > single rmapbt -- if the map/unmap operations apply to different AGs, no > > confusion will result from out-of-order execution. > > *nod* > > > > I'd suggest that the list_sort compare function could take this into > > > account, and order overlapping operations appropriately (e.g. unmap > > > before map). You could even put a sequence number in the rmap list > > > entry so that when sorting we know what order they were added to the > > > rmap list and then overlaps could be resolved in the sort function > > > easily... > > > > I /think/ we're fine with just the list_add_tail... > > Yeah, it seems that way, but it'll need "here be dragons" comment > around it... Will do. > > ...if not, /me wonders if we could skip the list_sort and just roll the > > transaction every time we have to go from a higher AG to a lower AG. I think > > the rmap update intent log items will help us to redo the metadata operations > > if we crash. > > I think that would work, but we want to keep the number of > transactions to a minimum. Trying to keep everything grouped by AG > definitely helps wiht that. What would be nice to know is how many > operations we are expecting to see here will actually have extents > from multiple AGs in them, and hence how many transactions are the > likely common case... Ok, I'll add some instrumentation so I can keep tabs on the number of intents per freelist and the number of AGs that get touched per freelist. My current guess is that we usually only have one or two rmaps to update per transaction. (I'm not even sure doing a xfs_trans_roll per AG is necessary; so far in my testing I've never come close to exceeding the log reservation for the rmap step. But that's before we start logging rmap intents. :)) Ok sleep now. --D > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
