On 6/20/16 5:57 PM, Dave Chinner wrote:
> On Sat, Jun 18, 2016 at 01:15:10PM -0700, Darrick J. Wong wrote:
>> On Fri, Jun 17, 2016 at 04:59:30AM -0700, Christoph Hellwig wrote:
>>>> {
>>>> + struct xfs_bmap_free_item *new; /* new element */
>>>> #ifdef DEBUG
>>>> xfs_agnumber_t agno;
>>>> xfs_agblock_t agbno;
>>>> @@ -597,17 +595,7 @@ xfs_bmap_add_free(
>>>> new = kmem_zone_alloc(xfs_bmap_free_item_zone, KM_SLEEP);
>>>> new->xbfi_startblock = bno;
>>>> new->xbfi_blockcount = (xfs_extlen_t)len;
>>>> + list_add(&new->xbfi_list, &flist->xbf_flist);
>>>> flist->xbf_count++;
>>>
>>> Please kill xbf_count while you're at it, it's entirely superflous.
>>
>> The deferred ops conversion patch kills this off by moving the whole
>> "defer an op to the next transaction by logging redo items" logic
>> into a separate file and mechanism.
>>
>> This patch is just a cleanup to reduce some of the open coded list ugliness
>> before starting on the rmap stuff. Once the deferred ops code lands, all
>> three of these functions go away.
>
> Ok, so because all these functions go away, I'll take this patch now
> without the suggested cleanups so that you don't have to rework it.
>
> ....
>
>>>> + list_sort((*tp)->t_mountp, &flist->xbf_flist, xfs_bmap_free_list_cmp);
>>>
>>> Can you add a comment on why we are sorting the list?
>>
>> We sort the list so that we process the freed extents in AG order to
>> avoid deadlocking.
>>
>> I'll add a comment to the deferred ops code if there isn't one already.
>
> This seems best - add the clean up to the later patches rather than
> have to rework lots of patches because of minor mods to early
> patches...
I'm woefully late to the game here, but adding comments later doesn't
help a future reader of commits like this, when neither the commit log
nor the patch contents explains things like why it's being sorted.
> then use list_sort to sort it prior to processing.
The changelog should say *why* a thing was done, not just *what* was
done; the diff already tells us that, right...
</soapbox>
-Eric
|