Re: [PATCH 3/4] btrfs: fix race when defragging that leads to unnecessary IO

[Josef Bacik <josef@toxicpanda.com>]

On 11/4/20 6:07 AM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When defragging we skip ranges that have holes or inline extents, so that
> we don't do unnecessary IO and waste space. We do this check when calling
> should_defrag_range() at btrfs_defrag_file(). However we do it without
> holding the inode's lock. The reason we do it like this is to avoid
> blocking other tasks for too long, that possibly want to operate on other
> file ranges, since after the call to should_defrag_range() and before
> locking the inode, we trigger a synchronous page cache readahead. However
> before we were able to lock the inode, some other task might have punched
> a hole in our range, or we may now have an inline extent there, in which
> case we should not set the range for defrag anymore since that would cause
> unnecessary IO and make us waste space (i.e. allocating extents to contain
> zeros for a hole).
> 
> So after we locked the inode and the range in the iotree, check again if
> we have holes or an inline extent, and if we do, just skip the range.
> 
> I hit this while testing my next patch that fixes races when updating an
> inode's number of bytes (subject "btrfs: update the number of bytes used
> by an inode atomically"), and it depends on this change in order to work
> correctly. Alternatively I could rework that other patch to detect holes
> and flag their range with the 'new delalloc' bit, but this itself fixes
> an efficiency problem due a race that from a functional point of view is
> not harmful (it could be triggered with btrfs/062 from fstests).
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Thanks,

Josef