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: Y8RIdcTS8gvI2m/EC1mBkQ6jJTkv+Za/LTTyhnwjXNBcZ/jvZ8fWO/xeClb45LCqpoi0Mfk+2+
 Dc2ghQ6T3J+xlW7O0XES8BwuoJkVNIV0d3OMXOFD8lmSX3e9PYEs2aOnLXnr8p/MKfqFy4BZJT
 ZFSEWtJYW1W3Orysyrjw/U9rb66IoFANWUd9Y/CCjKQ7yoZ6r+4+CqspYgoz416Q7TVyd37uwn
 vqrRDbZuh4xnQoKhQ5EFXa1i1ChmgRzsZtW+Hwc8rAx/uVIBAwjU04zY+eRG1tMbTJP/ip6EAh
 9gs=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     =?iso-8859-1?Q?Javier_Gonz=E1lez?= <javier@javigon.com>
CC:     "jaegeuk@kernel.org" <jaegeuk@kernel.org>,
        "yuchao0@huawei.com" <yuchao0@huawei.com>,
        "linux-f2fs-devel@lists.sourceforge.net" 
        <linux-f2fs-devel@lists.sourceforge.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        =?iso-8859-1?Q?Javier_Gonz=E1lez?= <javier.gonz@samsung.com>
Subject: Re: [PATCH] f2fs: disble physical prealloc in LSF mount
Thread-Topic: [PATCH] f2fs: disble physical prealloc in LSF mount
Thread-Index: AQHVoRNC1jbxwLDWckincxKmSLcqwA==
Date:   Tue, 26 Nov 2019 02:06:53 +0000
Message-ID: <BYAPR04MB58161C14246FA30366B69B9DE7450@BYAPR04MB5816.namprd04.prod.outlook.com>
References: <20191122085952.12754-1-javier@javigon.com>
 <BYAPR04MB58166AE029D919C6610D8404E74A0@BYAPR04MB5816.namprd04.prod.outlook.com>
 <20191125190320.g7beal27nc5ubju7@mpHalley>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 18423625-5152-4a53-41b4-08d772154e7e
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Nov 2019 02:06:53.1502
 (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: nzSoaKo4uacd0QkF3EGT5ZShXauYlgj083OyRjZjXhKwIstqbTr5yGy3OE3RJrwTySTZu9BtytUpsFxKqLQBVw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5608
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 4:03, Javier Gonz=E1lez wrote:=0A=
> On 25.11.2019 00:48, Damien Le Moal wrote:=0A=
>> On 2019/11/22 18:00, Javier Gonz=E1lez wrote:=0A=
>>> From: Javier Gonz=E1lez <javier.gonz@samsung.com>=0A=
>>>=0A=
>>> Fix file system corruption when using LFS mount (e.g., in zoned=0A=
>>> devices). Seems like the fallback into buffered I/O creates an=0A=
>>> inconsistency if the application is assuming both read and write DIO. I=
=0A=
>>> can easily reproduce a corruption with a simple RocksDB test.=0A=
>>>=0A=
>>> Might be that the f2fs_forced_buffered_io path brings some problems too=
,=0A=
>>> but I have not seen other failures besides this one.=0A=
>>>=0A=
>>> Problem reproducible without a zoned block device, simply by forcing=0A=
>>> LFS mount:=0A=
>>>=0A=
>>>   $ sudo mkfs.f2fs -f -m /dev/nvme0n1=0A=
>>>   $ sudo mount /dev/nvme0n1 /mnt/f2fs=0A=
>>>   $ sudo  /opt/rocksdb/db_bench  --benchmarks=3Dfillseq --use_existing_=
db=3D0=0A=
>>>   --use_direct_reads=3Dtrue --use_direct_io_for_flush_and_compaction=3D=
true=0A=
>>>   --db=3D/mnt/f2fs --num=3D5000 --value_size=3D1048576 --verify_checksu=
m=3D1=0A=
>>>   --block_size=3D65536=0A=
>>>=0A=
>>> Note that the options that cause the problem are:=0A=
>>>   --use_direct_reads=3Dtrue --use_direct_io_for_flush_and_compaction=3D=
true=0A=
>>>=0A=
>>> Fixes: f9d6d0597698 ("f2fs: fix out-place-update DIO write")=0A=
>>>=0A=
>>> Signed-off-by: Javier Gonz=E1lez <javier.gonz@samsung.com>=0A=
>>> ---=0A=
>>>  fs/f2fs/data.c | 3 ---=0A=
>>>  1 file changed, 3 deletions(-)=0A=
>>>=0A=
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c=0A=
>>> index 5755e897a5f0..b045dd6ab632 100644=0A=
>>> --- a/fs/f2fs/data.c=0A=
>>> +++ b/fs/f2fs/data.c=0A=
>>> @@ -1081,9 +1081,6 @@ int f2fs_preallocate_blocks(struct kiocb *iocb, s=
truct iov_iter *from)=0A=
>>>  			return err;=0A=
>>>  	}=0A=
>>>=0A=
>>> -	if (direct_io && allow_outplace_dio(inode, iocb, from))=0A=
>>> -		return 0;=0A=
>>=0A=
>> Since for LFS mode, all DIOs can end up out of place, I think that it=0A=
>> may be better to change allow_outplace_dio() to always return true in=0A=
>> the case of LFS mode. So may be something like:=0A=
>>=0A=
>> static inline int allow_outplace_dio(struct inode *inode,=0A=
>> 			struct kiocb *iocb, struct iov_iter *iter)=0A=
>> {=0A=
>> 	struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);=0A=
>> 	int rw =3D iov_iter_rw(iter);=0A=
>>=0A=
>> 	return test_opt(sbi, LFS) ||=0A=
>> 	 	(rw =3D=3D WRITE && !block_unaligned_IO(inode, iocb, iter));=0A=
>> }=0A=
>>=0A=
>> instead of the original:=0A=
>>=0A=
>> static inline int allow_outplace_dio(struct inode *inode,=0A=
>> 			struct kiocb *iocb, struct iov_iter *iter)=0A=
>> {=0A=
>> 	struct f2fs_sb_info *sbi =3D F2FS_I_SB(inode);=0A=
>> 	int rw =3D iov_iter_rw(iter);=0A=
>>=0A=
>> 	return (test_opt(sbi, LFS) && (rw =3D=3D WRITE) &&=0A=
>> 				!block_unaligned_IO(inode, iocb, iter));=0A=
>> }=0A=
>>=0A=
>> Thoughts ?=0A=
>>=0A=
> =0A=
> I see what you mean and it makes sense. However, the problem I am seeing=
=0A=
> occurs when allow_outplace_dio() returns true, as this is what creates=0A=
> the inconsistency between the write being buffered and the read being=0A=
> DIO.=0A=
=0A=
But if the write is switched to buffered, the DIO read should use the=0A=
buffered path too, no ? Since this is all happening under VFS, the=0A=
generic DIO read path will not ensure that the buffered writes are=0A=
flushed to disk before issuing the direct read, I think. So that would=0A=
explain your data corruption, i.e. you are reading stale data on the=0A=
device before the buffered writes make it to the media.=0A=
=0A=
> =0A=
> I did test the code you propose and, as expected, it still triggered the=
=0A=
> corruption.=0A=
> =0A=
>>> -=0A=
>>>  	if (is_inode_flag_set(inode, FI_NO_PREALLOC))=0A=
>>>  		return 0;=0A=
>>>=0A=
>>>=0A=
>>=0A=
>>=0A=
>> -- =0A=
>> Damien Le Moal=0A=
>> Western Digital Research=0A=
> =0A=
> Thanks,=0A=
> Javier=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=