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: JLPrOr0w3sDmRjHVNeAw9HbpJOw8Z0fCtaVwf+3ils2GBcvVuiqcLan5LwzIFlOOcIpuskUSq1
 Ab7lacOx5oIu+okQ/p7yWecygMxNdsufUFTx9gBg8LMsv5olNXlTa7eBkv2lw/zTOh5yRiDlLm
 jZejvt8Zfp3b+hRjntyOSWtcO/KeGkcSLwZICSKMDn2VuDAy2FcSsstee0fzeqcfJvOStiKEHJ
 QwyJAz6qnj+VeqyhjokOoG/72xLAhRbwVPa/fGk1VlhRP5BHPLHQF35dVX2yOzduRwtAhfl3JF
 OVI=
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>,
        Shinichiro Kawasaki <shinichiro.kawasaki@wdc.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 06:20:30 +0000
Message-ID: <BYAPR04MB5816F0BB42891E49C5AB42DDE7450@BYAPR04MB5816.namprd04.prod.outlook.com>
References: <20191122085952.12754-1-javier@javigon.com>
 <BYAPR04MB58166AE029D919C6610D8404E74A0@BYAPR04MB5816.namprd04.prod.outlook.com>
 <20191125190320.g7beal27nc5ubju7@mpHalley>
 <BYAPR04MB58161C14246FA30366B69B9DE7450@BYAPR04MB5816.namprd04.prod.outlook.com>
 <20191126035726.xj7pierxsck6adow@mpHalley>
 <BYAPR04MB581676157DCF909EDF1AAAFCE7450@BYAPR04MB5816.namprd04.prod.outlook.com>
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: 140074a5-3535-4a9e-6e13-08d77238bcc5
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Nov 2019 06:20:30.4636
 (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: SylEVGrexVPdMqOKZVt7bU5qgD4AOubzFOy4P26ygoauIYQ0lKqtaZ1nR3zKHaSZkZWzv8VV1T1UH/efoGXlEQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4629
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

+ Shin'Ichiro=0A=
=0A=
On 2019/11/26 15:19, Damien Le Moal wrote:=0A=
> On 2019/11/26 12:58, Javier Gonz=E1lez wrote:=0A=
>> On 26.11.2019 02:06, Damien Le Moal wrote:=0A=
>>> 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_existi=
ng_db=3D0=0A=
>>>>>>   --use_direct_reads=3Dtrue --use_direct_io_for_flush_and_compaction=
=3Dtrue=0A=
>>>>>>   --db=3D/mnt/f2fs --num=3D5000 --value_size=3D1048576 --verify_chec=
ksum=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=
=3Dtrue=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=
, struct 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 seei=
ng=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=
>> As far as I can see, the read is always sent DIO, so yes, I also believe=
=0A=
>> that we are reading stale data. This is why the corruption is not seen=
=0A=
>> if preventing allow_outplace_dio() from sending the write to the=0A=
>> buffered path.=0A=
>>=0A=
>> What surprises me is that this is very easy to trigger (see commit), so=
=0A=
>> I assume you must have seen this with SMR in the past.=0A=
> =0A=
> We just did. Shin'Ichiro in my team finally succeeded in recreating the=
=0A=
> problem. The cause seems to be:=0A=
> =0A=
> bool direct_io =3D iocb->ki_flags & IOCB_DIRECT;=0A=
> =0A=
> being true on entry of f2fs_preallocate_blocks() whereas=0A=
> f2fs_direct_IO() forces buffered IO path for DIO on zoned devices with:=
=0A=
> =0A=
> if (f2fs_force_buffered_io(inode, iocb, iter))=0A=
> 		return 0;=0A=
> =0A=
> which has:=0A=
> =0A=
> 	if (f2fs_sb_has_blkzoned(sbi))=0A=
> 		return true;=0A=
> =0A=
> So the top DIO code says "do buffered IOs", but lower in the write path,=
=0A=
> the IO is still assumed to be a DIO because of the iocb flag... That's=0A=
> inconsistent.=0A=
> =0A=
> Note that for the non-zoned device LFS case, f2fs_force_buffered_io()=0A=
> returns true only for unaligned write DIOs... But that will still trip=0A=
> on the iocb flag test. So the proper fix is likely something like:=0A=
> =0A=
> int f2fs_preallocate_blocks(struct kiocb *iocb, struct iov_iter *from)=0A=
> {=0A=
> 	struct inode *inode =3D file_inode(iocb->ki_filp);=0A=
> 	struct f2fs_map_blocks map;=0A=
> 	int flag;=0A=
> 	int err =3D 0;=0A=
> -	bool direct_io =3D iocb->ki_flags & IOCB_DIRECT;=0A=
> +	bool direct_io =3D (iocb->ki_flags & IOCB_DIRECT) &&=0A=
> +		!2fs_force_buffered_io(inode, iocb, iter);=0A=
> =0A=
> 	/* convert inline data for Direct I/O*/=0A=
> 	if (direct_io) {=0A=
> 		err =3D f2fs_convert_inline_inode(inode);=0A=
> 		if (err)=0A=
> 			return err;=0A=
> 	}=0A=
> =0A=
> Shin'Ichiro tried this on SMR disks and the failure is gone...=0A=
> =0A=
> Cheers.=0A=
> =0A=
> =0A=
>>=0A=
>> Does it make sense to leave the LFS check out of the=0A=
>> allow_outplace_dio()? Or in other words, is there a hard requirement for=
=0A=
>> writes to take this path on a zoned device that I am not seeing?=0A=
>> 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 (rw =3D=3D WRITE && !block_unaligned_IO(inode, iocb, iter));=
=0A=
>>    }=0A=
>>=0A=
>> Thanks,=0A=
>> Javier=0A=
>>=0A=
> =0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=