Re: [PATCH v4 0/4] ceph: size handling for the fscrypt

From: Xiubo Li <xiubli@redhat.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: idryomov@gmail.com, vshankar@redhat.com, pdonnell@redhat.com,
	khiremat@redhat.com, ceph-devel@vger.kernel.org
Subject: Re: [PATCH v4 0/4] ceph: size handling for the fscrypt
Date: Tue, 2 Nov 2021 09:02:39 +0800	[thread overview]
Message-ID: <402e819c-2e80-a09f-2e92-988cc90ff9b8@redhat.com> (raw)
In-Reply-To: <b4db9a2377711857118d1fbd5835b7b8d7c9019c.camel@kernel.org>

On 11/2/21 1:07 AM, Jeff Layton wrote:
> On Mon, 2021-11-01 at 06:27 -0400, Jeff Layton wrote:
>> On Mon, 2021-11-01 at 10:04 +0800, xiubli@redhat.com wrote:
>>> From: Xiubo Li <xiubli@redhat.com>
>>>
>>> This patch series is based on the "fscrypt_size_handling" branch in
>>> https://github.com/lxbsz/linux.git, which is based Jeff's
>>> "ceph-fscrypt-content-experimental" branch in
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git
>>> and added two upstream commits, which should be merged already.
>>>
>>> These two upstream commits should be removed after Jeff rebase
>>> his "ceph-fscrypt-content-experimental" branch to upstream code.
>>>
>> I don't think I was clear last time. I'd like for you to post the
>> _entire_ stack of patches that is based on top of
>> ceph-client/wip-fscrypt-fnames. wip-fscrypt-fnames is pretty stable at
>> this point, so I think it's a reasonable place for you to base your
>> work. That way you're not beginning with a revert.
>>
>> Again, feel free to cherry-pick any of the patches in any of my other
>> branches for your series, but I'd like to see a complete series of
>> patches.
>>
>>
> To be even more clear:
>
> The main reason this patchset is not helpful is that the
> ceph-fscrypt-content-experimental branch in my tree has bitrotted in the
> face of other changes that have gone into the testing branch since it
> was cut. Also, that branch had several patches that added in actual
> encryption of the content, which we don't want to do at this point.
>
> For the work you're doing, what I'd like to see is a patchset based on
> top of the ceph-client/wip-fscrypt-fnames branch. That patchset should
> make it so what when encryption is enabled, the size handling for the
> inode is changed to use the new scheme we've added, but don't do any
> actual content encryption yet. Feel free to pick any of the patches in
> my trees that you need to do this, but it needs to be the whole series.
>
> What we need to be able to do in this phase is validate that the size
> handling is correct in both the encrypted and non-encrypted cases, but
> without encrypting the data. Once that piece is working, then we should
> be able to add in content encryption.

Okay, get it.

I will cleanup the code and respin it later.

BRs

-- Xiubo

>
>>> ====
>>>
>>> This approach is based on the discussion from V1 and V2, 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().
>>>
>>> This just assume kclient won't support the inline data feature, which
>>> will be remove soon, more detail please see:
>>> https://tracker.ceph.com/issues/52916
>>>
>>> Changed in V4:
>>> - Retry the truncate request by 20 times before fail it with -EAGAIN.
>>> - Remove the "fill_last_block" label and move the code to else branch.
>>> - Remove the #3 patch, which has already been sent out separately, in
>>>    V3 series.
>>> - Improve some comments in the code.
>>>
>>>
>>> Changed in V3:
>>> - Fix possibly corrupting the file just before the MDS acquires the
>>>    xlock for FILE lock, another client has updated it.
>>> - Flush the pagecache buffer before reading the last block for the
>>>    when filling the truncate request.
>>> - Some other minore fixes.
>>>
>>> Xiubo Li (4):
>>>    Revert "ceph: make client zero partial trailing block on truncate"
>>>    ceph: add __ceph_get_caps helper support
>>>    ceph: add __ceph_sync_read helper support
>>>    ceph: add truncate size handling support for fscrypt
>>>
>>>   fs/ceph/caps.c              |  21 ++--
>>>   fs/ceph/file.c              |  44 +++++---
>>>   fs/ceph/inode.c             | 203 ++++++++++++++++++++++++++++++------
>>>   fs/ceph/super.h             |   6 +-
>>>   include/linux/ceph/crypto.h |  28 +++++
>>>   5 files changed, 251 insertions(+), 51 deletions(-)
>>>   create mode 100644 include/linux/ceph/crypto.h
>>>