Xiubo Li <xiubli@xxxxxxxxxx> writes: > On 4/6/22 6:57 PM, Luís Henriques wrote: >> Xiubo Li <xiubli@xxxxxxxxxx> writes: >> >>> On 4/1/22 9:32 PM, Luís Henriques wrote: >>>> When doing DIO on an encrypted node, we need to invalidate the page cache in >>>> the range being written to, otherwise the cache will include invalid data. >>>> >>>> Signed-off-by: Luís Henriques <lhenriques@xxxxxxx> >>>> --- >>>> fs/ceph/file.c | 11 ++++++++++- >>>> 1 file changed, 10 insertions(+), 1 deletion(-) >>>> >>>> Changes since v1: >>>> - Replaced truncate_inode_pages_range() by invalidate_inode_pages2_range >>>> - Call fscache_invalidate with FSCACHE_INVAL_DIO_WRITE if we're doing DIO >>>> >>>> Note: I'm not really sure this last change is required, it doesn't really >>>> affect generic/647 result, but seems to be the most correct. >>>> >>>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >>>> index 5072570c2203..b2743c342305 100644 >>>> --- a/fs/ceph/file.c >>>> +++ b/fs/ceph/file.c >>>> @@ -1605,7 +1605,7 @@ ceph_sync_write(struct kiocb *iocb, struct iov_iter *from, loff_t pos, >>>> if (ret < 0) >>>> return ret; >>>> - ceph_fscache_invalidate(inode, false); >>>> + ceph_fscache_invalidate(inode, (iocb->ki_flags & IOCB_DIRECT)); >>>> ret = invalidate_inode_pages2_range(inode->i_mapping, >>>> pos >> PAGE_SHIFT, >>>> (pos + count - 1) >> PAGE_SHIFT); >>> The above has already invalidated the pages, why doesn't it work ? >> I suspect the reason is because later on we loop through the number of >> pages, call copy_page_from_iter() and then ceph_fscrypt_encrypt_pages(). > > Checked the 'copy_page_from_iter()', it will do the kmap for the pages but will > kunmap them again later. And they shouldn't update the i_mapping if I didn't > miss something important. > > For 'ceph_fscrypt_encrypt_pages()' it will encrypt/dencrypt the context inplace, > IMO if it needs to map the page and it should also unmap it just like in > 'copy_page_from_iter()'. > > I thought it possibly be when we need to do RMW, it may will update the > i_mapping when reading contents, but I checked the code didn't find any > place is doing this. So I am wondering where tha page caches come from ? If that > page caches really from reading the contents, then we should discard it instead > of flushing it back ? > > BTW, what's the problem without this fixing ? xfstest fails ? Yes, generic/647 fails if you run it with test_dummy_encryption. And I've also checked that the RMW code was never executed in this test. But yeah I have assumed (perhaps wrongly) that the kmap/kunmap could change the inode->i_mapping. In my debugging this seemed to be the case for the O_DIRECT path. That's why I added this extra call here. Cheers, -- Luís
