| Message ID | 20220401113822.32545-1-lhenriques@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: truncate page cache when doing DIO in encrypted inodes | expand |
On Fri, 2022-04-01 at 12:38 +0100, Luís Henriques wrote: > When doing DIO on an encrypted node, we need to truncate the page cache in > the range being written to, otherwise the cache will include invalid data. > > Signed-off-by: Luís Henriques <lhenriques@suse.de> > --- > fs/ceph/file.c | 5 +++++ > 1 file changed, 5 insertions(+) > > This patch should fix generic/647 fstest when run with test_dummy_encryption. > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 5072570c2203..0f31c4d352a4 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1895,6 +1895,11 @@ ceph_sync_write(struct kiocb *iocb, struct iov_iter *from, loff_t pos, > req->r_inode = inode; > req->r_mtime = mtime; > > + if (IS_ENCRYPTED(inode) && (iocb->ki_flags & IOCB_DIRECT)) > + truncate_inode_pages_range( > + inode->i_mapping, write_pos, > + PAGE_ALIGN(write_pos + write_len) - 1); > + > /* Set up the assertion */ > if (rmw) { > /* Truncating the pagecache like this could cause dirty data to be discarded. I know we're planning to overwrite this range, but you are having to invalidate more than the written range here. We could potentially lose a write to that region. Have you tried using something like invalidate_inode_pages2_range ? That's more of what we'd want here, as it's a bit more cautious about tossing out dirty pages. I see too that that is what ceph_direct_read_write calls in the write case as well.
Jeff Layton <jlayton@kernel.org> writes: > On Fri, 2022-04-01 at 12:38 +0100, Luís Henriques wrote: >> When doing DIO on an encrypted node, we need to truncate the page cache in >> the range being written to, otherwise the cache will include invalid data. >> >> Signed-off-by: Luís Henriques <lhenriques@suse.de> >> --- >> fs/ceph/file.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> This patch should fix generic/647 fstest when run with test_dummy_encryption. >> >> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> index 5072570c2203..0f31c4d352a4 100644 >> --- a/fs/ceph/file.c >> +++ b/fs/ceph/file.c >> @@ -1895,6 +1895,11 @@ ceph_sync_write(struct kiocb *iocb, struct iov_iter *from, loff_t pos, >> req->r_inode = inode; >> req->r_mtime = mtime; >> >> + if (IS_ENCRYPTED(inode) && (iocb->ki_flags & IOCB_DIRECT)) >> + truncate_inode_pages_range( >> + inode->i_mapping, write_pos, >> + PAGE_ALIGN(write_pos + write_len) - 1); >> + >> /* Set up the assertion */ >> if (rmw) { >> /* > > Truncating the pagecache like this could cause dirty data to be > discarded. I know we're planning to overwrite this range, but you are > having to invalidate more than the written range here. We could > potentially lose a write to that region. > > Have you tried using something like invalidate_inode_pages2_range ? > That's more of what we'd want here, as it's a bit more cautious about > tossing out dirty pages. I see too that that is what > ceph_direct_read_write calls in the write case as well. OK, let me try that instead. Yeah, I've used what the usual direct path was using (the truncate_inode_pages_range()), but if invalidate_inode_pages2_range works here I guess that's better. I'll test that and send out v2. Cheers,
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 5072570c2203..0f31c4d352a4 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1895,6 +1895,11 @@ ceph_sync_write(struct kiocb *iocb, struct iov_iter *from, loff_t pos, req->r_inode = inode; req->r_mtime = mtime; + if (IS_ENCRYPTED(inode) && (iocb->ki_flags & IOCB_DIRECT)) + truncate_inode_pages_range( + inode->i_mapping, write_pos, + PAGE_ALIGN(write_pos + write_len) - 1); + /* Set up the assertion */ if (rmw) { /*
When doing DIO on an encrypted node, we need to truncate the page cache in the range being written to, otherwise the cache will include invalid data. Signed-off-by: Luís Henriques <lhenriques@suse.de> --- fs/ceph/file.c | 5 +++++ 1 file changed, 5 insertions(+) This patch should fix generic/647 fstest when run with test_dummy_encryption.