Re: [PATCH] btrfs: introduce tickets_id to determine whether asynchronous metadata reclaim work makes progress

[Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>]

hello,

On 09/02/2016 09:28 PM, Josef Bacik wrote:
> On 09/01/2016 10:58 PM, Wang Xiaoguang wrote:
>> In btrfs_async_reclaim_metadata_space(), we use ticket's address to
>> determine whether asynchronous metadata reclaim work is making progress.
>>
>>     ticket = list_first_entry(&space_info->tickets,
>>                   struct reserve_ticket, list);
>>     if (last_ticket == ticket) {
>>         flush_state++;
>>     } else {
>>         last_ticket = ticket;
>>         flush_state = FLUSH_DELAYED_ITEMS_NR;
>>         if (commit_cycles)
>>             commit_cycles--;
>>     }
>>
>> But indeed it's wrong, we should not rely on local variable's address to
>> do this check, because addresses may be same. In my test environment, I
>> dd one 168MB file in a 256MB fs, found that for this file, every time
>> wait_reserve_ticket() called, local variable ticket's address is same,
>>
>> For above codes, assume a previous ticket's address is addrA, 
>> last_ticket
>> is addrA. Btrfs_async_reclaim_metadata_space() finished this ticket and
>> wake up it, then another ticket is added, but with the same address 
>> addrA,
>> now last_ticket will be same to current ticket, then current ticket's 
>> flush
>> work will start from current flush_state, not initial 
>> FLUSH_DELAYED_ITEMS_NR,
>> which may result in some enospc issues(I have seen this in my test 
>> machine).
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>
> We want to track the progress of the individual tickets, not whether 
> or not we make progress on the space_info, so instead store the global 
> ticket id in space_info and store the individual ticket_id in the 
> ticket itself, and use that as the last_tickets_id.  Thanks,
Sorry for being late.

In btrfs_async_reclaim_metadata_space(), there is codes:
if (flush_state > COMMIT_TRANS) {
             commit_cycles++;
             if (commit_cycles > 2) {
                 wake_all_tickets(&space_info->tickets);
                 space_info->flush = 0;
             } else {
                 flush_state = FLUSH_DELAYED_ITEMS_NR;
             }
}
 From above codes, if it can not satisfy one current ticket, it'll 
discard all current
queued tickets. So I think your original codes is to track whether or not we
make progress on the space_info, and this patch can fix the issue which 
is described
in commit message.
Also I'm not sure whether I have understood your design completely. If 
I'm wrong, sorry.

Regards,
Xiaoguang Wang

>
> Josef
>
>