On 09/06/2016 06:18 AM, Wang Xiaoguang wrote:
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 <[email protected]>
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.
Sorry I misread your patch, I thought you were doing space_info->ticket_id++
when you added a ticket to the list, not when you removed it. You can add
Reviewed-by: Josef Bacik <[email protected]>
Thanks,
Josef
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
