Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
IronPort-SDR: ovf/sGAGaOWIvD0gyXG3a+UT/RDtRYaEPxxRMDWJbj5TaYpnP4xgqexOcR/axJ7ATYkn1NHDTF
 ALLzBqtWnZT6QQr38qi0SnDM0L7vMeTnoCMiLeeFgseiC2jlj5tW7cr7mFEIyA4RcffHSXZEBT
 6vZ84KubqDBvEOJlwMDru/e0QYJVWD37Q/lPYYv1nDMz6J3XX0rZNCU1otXqlhJb39EGytPtEq
 SNAc7PrOjdqc1o+ENOzZQv/H7eJ8cR9YvXzpxfemsAQXTFezHQ1D8kENfi73b3UxIfkvS9Cffh
 yXU=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Ritesh Harjani <riteshh@linux.ibm.com>,
        "linux-f2fs-devel@lists.sourceforge.net" 
        <linux-f2fs-devel@lists.sourceforge.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Jaegeuk Kim <jaegeuk@kernel.org>, Chao Yu <yuchao0@huawei.com>
CC:     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        Javier Gonzalez <javier@javigon.com>,
        Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Subject: Re: [PATCH] f2fs: Fix direct IO handling
Thread-Topic: [PATCH] f2fs: Fix direct IO handling
Thread-Index: AQHVpC8oKo+FZgLUZUKdgZnQSrVbXw==
Date:   Thu, 28 Nov 2019 02:10:28 +0000
Message-ID: <BYAPR04MB5816C82F708612381216895BE7470@BYAPR04MB5816.namprd04.prod.outlook.com>
References: <20191126075719.1046485-1-damien.lemoal@wdc.com>
 <20191126083443.F1FD5A405B@b06wcsmtp001.portsmouth.uk.ibm.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5574de00-208c-4881-e394-08d773a823ad
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Nov 2019 02:10:28.4204
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 8gqRIFPT42ai7KNCMu7drCOZFpf0uZzjtXlWbw332nulvHWZIFmVrTVu0T1o5TXsWG3tf/pGyJkgNh8CCCOzwA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5718
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2019/11/26 17:34, Ritesh Harjani wrote:=0A=
> Hello Damien,=0A=
> =0A=
> IIUC, you are trying to fix a stale data read by DIO read for the case=0A=
> you explained in your patch w.r.t. DIO-write forced to write as buffIO.=
=0A=
> =0A=
> Coincidentally I was just looking at the same code path just now.=0A=
> So I do have a query to you/f2fs group. Below could be silly one, as I=0A=
> don't understand F2FS in great detail.=0A=
> =0A=
> How is the stale data by DIO read, is protected against a mmap=0A=
> writes via f2fs_vm_page_mkwrite?=0A=
> =0A=
> f2fs_vm_page_mkwrite()		 f2fs_direct_IO (read)=0A=
> 					filemap_write_and_wait_range()=0A=
> 	-> f2fs_get_blocks()				=0A=
> 					 -> submit_bio()=0A=
> =0A=
> 	-> set_page_dirty()=0A=
> =0A=
> Is above race possible with current f2fs code?=0A=
> i.e. f2fs_direct_IO could read the stale data from the blocks=0A=
> which were allocated due to mmap fault?=0A=
=0A=
The faulted page is locked until the fault is fully processed so direct=0A=
IO has to wait for that to complete first.=0A=
=0A=
> =0A=
> Am I missing something here?=0A=
> =0A=
> -ritesh=0A=
> =0A=
> On 11/26/19 1:27 PM, Damien Le Moal wrote:=0A=
>> f2fs_preallocate_blocks() identifies direct IOs using the IOCB_DIRECT=0A=
>> flag for a kiocb structure. However, the file system direct IO handler=
=0A=
>> function f2fs_direct_IO() may have decided that a direct IO has to be=0A=
>> exececuted as a buffered IO using the function f2fs_force_buffered_io().=
=0A=
>> This is the case for instance for volumes including zoned block device=
=0A=
>> and for unaligned write IOs with LFS mode enabled.=0A=
>>=0A=
>> These 2 different methods of identifying direct IOs can result in=0A=
>> inconsistencies generating stale data access for direct reads after a=0A=
>> direct IO write that is treated as a buffered write. Fix this=0A=
>> inconsistency by combining the IOCB_DIRECT flag test with the result=0A=
>> of f2fs_force_buffered_io().=0A=
>>=0A=
>> Reported-by: Javier Gonzalez <javier@javigon.com>=0A=
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>=0A=
>> ---=0A=
>>   fs/f2fs/data.c | 4 +++-=0A=
>>   1 file changed, 3 insertions(+), 1 deletion(-)=0A=
>>=0A=
>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c=0A=
>> index 5755e897a5f0..8ac2d3b70022 100644=0A=
>> --- a/fs/f2fs/data.c=0A=
>> +++ b/fs/f2fs/data.c=0A=
>> @@ -1073,6 +1073,8 @@ int f2fs_preallocate_blocks(struct kiocb *iocb, st=
ruct iov_iter *from)=0A=
>>   	int flag;=0A=
>>   	int err =3D 0;=0A=
>>   	bool direct_io =3D iocb->ki_flags & IOCB_DIRECT;=0A=
>> +	bool do_direct_io =3D direct_io &&=0A=
>> +		!f2fs_force_buffered_io(inode, iocb, from);=0A=
>>   =0A=
>>   	/* convert inline data for Direct I/O*/=0A=
>>   	if (direct_io) {=0A=
>> @@ -1081,7 +1083,7 @@ int f2fs_preallocate_blocks(struct kiocb *iocb, st=
ruct iov_iter *from)=0A=
>>   			return err;=0A=
>>   	}=0A=
>>   =0A=
>> -	if (direct_io && allow_outplace_dio(inode, iocb, from))=0A=
>> +	if (do_direct_io && allow_outplace_dio(inode, iocb, from))=0A=
>>   		return 0;=0A=
>>   =0A=
>>   	if (is_inode_flag_set(inode, FI_NO_PREALLOC))=0A=
>>=0A=
> =0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=