On 10/26/21 11:41 AM, Xiubo Li wrote:
On 10/26/21 4:01 AM, Jeff Layton wrote:
On Wed, 2021-10-20 at 21:28 +0800, xiubli@xxxxxxxxxx wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>
This will transfer the encrypted last block contents to the MDS
along with the truncate request only when new size is smaller and
not aligned to the fscrypt BLOCK size.
Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
---
fs/ceph/caps.c | 9 +--
fs/ceph/inode.c | 210
++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 190 insertions(+), 29 deletions(-)
...
+fill_last_block:
+ pagelist = ceph_pagelist_alloc(GFP_KERNEL);
+ if (!pagelist)
+ return -ENOMEM;
+
+ /* Insert the header first */
+ header.ver = 1;
+ header.compat = 1;
+ /* sizeof(file_offset) + sizeof(block_size) + blen */
+ header.data_len = cpu_to_le32(8 + 8 + CEPH_FSCRYPT_BLOCK_SIZE);
+ header.file_offset = cpu_to_le64(orig_pos);
+ if (fill_header_only) {
+ header.file_offset = cpu_to_le64(0);
+ header.block_size = cpu_to_le64(0);
+ } else {
+ header.file_offset = cpu_to_le64(orig_pos);
+ header.block_size = cpu_to_le64(CEPH_FSCRYPT_BLOCK_SIZE);
+ }
+ ret = ceph_pagelist_append(pagelist, &header, sizeof(header));
+ if (ret)
+ goto out;
Note that you're doing a read/modify/write cycle, and you must ensure
that the object remains consistent between the read and write or you may
end up with data corruption. This means that you probably need to
transmit an object version as part of the write. See this patch in the
stack:
libceph: add CEPH_OSD_OP_ASSERT_VER support
That op tells the OSD to stop processing the request if the version is
wrong.
You'll want to grab the "ver" from the __ceph_sync_read call, and then
send that along with the updated last block. Then, when the MDS is
truncating, it can use a CEPH_OSD_OP_ASSERT_VER op with that version to
ensure that the object hasn't changed when doing it. If the assertion
trips, then the MDS should send back EAGAIN or something similar to the
client to tell it to retry.
Yeah, this is one issues I have mentioned in the cover-letter, I found
the cover-letter wasn't successfully sent to the mail list.
Except this, there also has another isssue, I will pasted some
important sections here from the cover-letter:
======
This approach is based on the discussion from V1, which will pass
the encrypted last block contents to MDS along with the truncate
request.
This will send the encrypted last block contents to MDS along with
the truncate request when truncating to a smaller size and at the
same time new size does not align to BLOCK SIZE.
The MDS side patch is raised in PR
https://github.com/ceph/ceph/pull/43588, which is also based Jeff's
previous great work in PR https://github.com/ceph/ceph/pull/41284.
The MDS will use the filer.write_trunc(), which could update and
truncate the file in one shot, instead of filer.truncate().
I have removed the inline data related code since we are remove
this feature, more detail please see:
https://tracker.ceph.com/issues/52916
Note: There still has two CORNER cases we need to deal with:
1), If a truncate request with the last block is sent to the MDS and
just before the MDS has acquired the xlock for FILE lock, if another
client has updated that last block content, we will over write the
last block with old data.
For this case we could send the old encrypted last block data along
with the truncate request and in MDS side read it and then do compare
just before updating it, if the comparasion fails, then fail the
truncate and let the kclient retry it.
2), If another client has buffered the last block, we should flush
it first. I am still thinking how to do this ? Any idea is welcome.
Checked more about the MDS locker related code, once we get the Fr caps
in the kclient which is doing the truncate, the Fb caps will be revoked
by the MDS already. It's not allowed that one client hold Fr cap, and
another client will hold the Fb. So we don't worry about the last block
will still be buffered by another kclient once we get the Fr caps.
But it's possible that if currently kclient is a loner, it could have
already buffered the last block in pagecache, since we need the
assert_ver from the Rados for the truncate, so we couldn't get the
contents from the pagecache and we need to flush the pagecache first and
read it again from Rados.
BRs
-- Xiubo
======
For the first issue, I will check the "CEPH_OSD_OP_ASSERT_VER".
For the second issue, I think the "CEPH_OSD_OP_ASSERT_VER" could work
too. When the MDS has successfully xlocked the FILE lock, we could
notify the kclients to flush the buffer first, and then try to
write/truncate the file, if it fails due to the
"CEPH_OSD_OP_ASSERT_VER" check, then let kclient retry the truncate
request.
NOTE: All the xlock and xlock related interim states allowed the
clients continue to hold the Fcb caps which are issued from previous
states, except that the LOCK_XLOCKSNAP only allow clients to hold Fc.
It's also possible (though I've never used it) to make an OSD request
assert that the contents of an extent haven't changed, so you could
instead send along the old contents along with the new ones, etc.
That may end up being more efficient if the object is getting hammered
with I/O in other fscrypt blocks within the same object. It may be worth
exploring that avenue as well.
+
+ if (!fill_header_only) {
+ /* Append the last block contents to pagelist */
+ for (i = 0; i < num_pages; i++) {
+ ret = ceph_pagelist_append(pagelist, iovs[i].iov_base,
+ blen);
+ if (ret)
+ goto out;
+ }
+ }
+ req->r_pagelist = pagelist;
+out:
+ dout("%s %p size dropping cap refs on %s\n", __func__,
+ inode, ceph_cap_string(got));
+ for (i = 0; iovs && i < num_pages; i++)
+ kunmap_local(iovs[i].iov_base);
+ kfree(iovs);
+ if (pages)
+ ceph_release_page_vector(pages, num_pages);
+ if (ret && pagelist)
+ ceph_pagelist_release(pagelist);
+ ceph_put_cap_refs(ci, got);
+ return ret;
+}
+
int __ceph_setattr(struct inode *inode, struct iattr *attr, struct
ceph_iattr *cia)
{
struct ceph_inode_info *ci = ceph_inode(inode);
@@ -2236,6 +2391,7 @@ int __ceph_setattr(struct inode *inode, struct
iattr *attr, struct ceph_iattr *c
struct ceph_mds_request *req;
struct ceph_mds_client *mdsc =
ceph_sb_to_client(inode->i_sb)->mdsc;
struct ceph_cap_flush *prealloc_cf;
+ loff_t isize = i_size_read(inode);
int issued;
int release = 0, dirtied = 0;
int mask = 0;
@@ -2367,10 +2523,31 @@ int __ceph_setattr(struct inode *inode,
struct iattr *attr, struct ceph_iattr *c
}
}
if (ia_valid & ATTR_SIZE) {
- loff_t isize = i_size_read(inode);
-
dout("setattr %p size %lld -> %lld\n", inode, isize,
attr->ia_size);
- if ((issued & CEPH_CAP_FILE_EXCL) && attr->ia_size >= isize) {
+ /*
+ * Only when the new size is smaller and not aligned to
+ * CEPH_FSCRYPT_BLOCK_SIZE will the RMW is needed.
+ */
+ if (IS_ENCRYPTED(inode) && attr->ia_size < isize &&
+ (attr->ia_size % CEPH_FSCRYPT_BLOCK_SIZE)) {
+ mask |= CEPH_SETATTR_SIZE;
+ release |= CEPH_CAP_FILE_SHARED | CEPH_CAP_FILE_EXCL |
+ CEPH_CAP_FILE_RD | CEPH_CAP_FILE_WR;
+ set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
+ mask |= CEPH_SETATTR_FSCRYPT_FILE;
+ req->r_args.setattr.size =
+ cpu_to_le64(round_up(attr->ia_size,
+ CEPH_FSCRYPT_BLOCK_SIZE));
+ req->r_args.setattr.old_size =
+ cpu_to_le64(round_up(isize,
+ CEPH_FSCRYPT_BLOCK_SIZE));
+ req->r_fscrypt_file = attr->ia_size;
+ spin_unlock(&ci->i_ceph_lock);
+ err = fill_request_for_fscrypt(inode, req, attr);
It'd be better to just defer the call to fill_request_for_fscrypt until
after you've dropped the spinlock, later in the function.
Sound good and will do it.
+ spin_lock(&ci->i_ceph_lock);
+ if (err)
+ goto out;
+ } else if ((issued & CEPH_CAP_FILE_EXCL) && attr->ia_size
>= isize) {
if (attr->ia_size > isize) {
i_size_write(inode, attr->ia_size);
inode->i_blocks = calc_inode_blocks(attr->ia_size);
@@ -2382,23 +2559,10 @@ int __ceph_setattr(struct inode *inode,
struct iattr *attr, struct ceph_iattr *c
attr->ia_size != isize) {
mask |= CEPH_SETATTR_SIZE;
release |= CEPH_CAP_FILE_SHARED | CEPH_CAP_FILE_EXCL |
- CEPH_CAP_FILE_RD | CEPH_CAP_FILE_WR;
- if (IS_ENCRYPTED(inode)) {
- set_bit(CEPH_MDS_R_FSCRYPT_FILE, &req->r_req_flags);
- mask |= CEPH_SETATTR_FSCRYPT_FILE;
- req->r_args.setattr.size =
- cpu_to_le64(round_up(attr->ia_size,
- CEPH_FSCRYPT_BLOCK_SIZE));
- req->r_args.setattr.old_size =
- cpu_to_le64(round_up(isize,
- CEPH_FSCRYPT_BLOCK_SIZE));
- req->r_fscrypt_file = attr->ia_size;
- /* FIXME: client must zero out any partial blocks! */
- } else {
- req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
- req->r_args.setattr.old_size = cpu_to_le64(isize);
- req->r_fscrypt_file = 0;
- }
+ CEPH_CAP_FILE_RD | CEPH_CAP_FILE_WR;
+ req->r_args.setattr.size = cpu_to_le64(attr->ia_size);
+ req->r_args.setattr.old_size = cpu_to_le64(isize);
+ req->r_fscrypt_file = 0;
}
}
if (ia_valid & ATTR_MTIME) {
