Re: [PATCH] btrfs: Fix locking during DIO read

[Nikolay Borisov <nborisov@suse.com>]

On 23.02.2018 01:39, David Sterba wrote:
> On Thu, Feb 22, 2018 at 12:24:40PM -0700, Liu Bo wrote:
>>>>> Not even that far, isize is truncated before calling inode_dio_wait()
>>>>> and a memory barrier is set to ensure the correct order, so dio read
>>>>> would simply return if it's reading past isize.
>>>>
>>>> Please, describe concretely which meory barriers pairs with chich in
>>>> order to ensure proper visibility. Because I'm of the opinion there are
>>>> insufficient memoru barriers to ensure setting READDIO_LOCK is properly
>>>> visible in btrfs_direct_IO. Since the latter has no barriers whatsoever.
>>>
>>> smp_mb() is supposed to be paired, so there is one missing, I agree.
>>>
>>
>> So the missing smp_mb() was there (commit
>> 2e60a51e62185cce48758e596ae7cb2da673b58f), but was removed in some
>> cleanup I guess.
> 
> Not a cleanup, and not a single commit:
> 
> 38851cc19adbfa1def2b47106d8050a80e0a3673
> Btrfs: serialize unlocked dio reads with truncate
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> 
> at this time the i_dio_counter was still used directly and the barrier
> was moved before the if-else block, technically still in the right
> before the test_bit.
> 
> dc59215d4f42084ee13654bafe3e5130b146aeb7
> btrfs: remove unnecessary memory barrier in btrfs_direct_IO
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> 
> ... simply removes the barrier.

Right, I removed this barrier because indeed it wasn't paired with
anything in inode_dio_wait. The barrier that is missing here has to
order accessing test_bit(READDIO_LOCK) and not ordering the atomic_inc
vs inode_dio_wait.

>