From: Gregory Farnum <gfarnum@redhat.com>
To: Xiubo Li <xiubli@redhat.com>
Cc: "Frank Hsiao 蕭法宣" <frankhsiao@qnap.com>,
"ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
"Jeff Layton" <jlayton@kernel.org>,
"Venky Shankar" <vshankar@redhat.com>,
"Patrick Donnelly" <pdonnell@redhat.com>
Subject: Re: Read operation gets EOF return when there is multi-client read/write after linux 5.16-rc1
Date: Wed, 21 Feb 2024 09:12:35 -0800 [thread overview]
Message-ID: <CAJ4mKGaSyNPMyCiM6WwyB9rAVGFr-_MYdwc5Q0k2YA4y0vOHWQ@mail.gmail.com> (raw)
In-Reply-To: <6f953a75-23cc-4d41-bbc5-2ca0a839f6d3@redhat.com>
On Mon, Feb 19, 2024 at 9:09 PM Xiubo Li <xiubli@redhat.com> wrote:
>
>
> On 1/24/24 11:25, Frank Hsiao 蕭法宣 wrote:
>
> When multiple ceph kernel clients perform read/write on the same file, the read
> operation(ceph_sync_read) returns EOF(ret = 0) even though the file has been
> written by another client.
>
> My envs use Ceph quincy(v17.2.6) and mount cephfs by ceph kernel client. For the
> client side, I use Samba(v4.18.8) to export the folder as smb share and test it
> with smbtorture. The test case is smb2.rw.rw1 with the following failure
> message:
>
> test: samba4.smb2.rw.rw1
> Checking data integrity over 10 ops
> read failed(NT_STATUS_END_OF_FILE)
> failure: samba4.smb2.rw.rw1 [
> Exception: read 0, expected 440
> ]
>
> After some testing, I figured out that the failure only happens when I have
> linux kernel version>=5.16-rc1, specifically after commit
> c3d8e0b5de487a7c462781745bc17694a4266696. Kernel logs as below(on 5.16-rc1):
>
>
> [Wed Jan 10 09:44:56 2024] [153221] ceph_read_iter:1559: ceph: aio_sync_read
> 00000000789dccee 100000010ef.fffffffffffffffe 0~440 got cap refs on Fr
> [Wed Jan 10 09:44:56 2024] [153221] ceph_sync_read:852: ceph: sync_read on file
> 00000000d9e861fb 0~440
> [Wed Jan 10 09:44:56 2024] [153221] ceph_sync_read:913: ceph: sync_read 0~440 got 440 i_size 0
> [Wed Jan 10 09:44:56 2024] [153221] ceph_sync_read:966: ceph: sync_read result 0 retry_op 2
>
> ...
>
> [Wed Jan 10 09:44:57 2024] [153221] ceph_read_iter:1559: ceph: aio_sync_read
> 00000000789dccee 100000010ef.fffffffffffffffe 0~440 got cap refs on Fr
> [Wed Jan 10 09:44:57 2024] [153221] ceph_sync_read:852: ceph: sync_read on file
> 00000000d9e861fb 0~0
>
>
> The logs indicate that:
> 1. ceph_sync_read may read data but i_size is obsolete in simultaneous rw situation
> 2. The commit in 5.16-rc1 cap ret to i_size and set retry_op = CHECK_EOF
> 3. When retrying, ceph_sync_read gets len=0 since iov count has modified in
> copy_page_to_iter
> 4. ceph_read_iter return 0
>
> Yeah, in simultaneious read and write case, the CInode's filelock will be in MIX mode. When one client append new contents to the file it will increase the 'i_size' metadata to MDS, and then the MDS will broadcast the new metadatas to all the other clients. But in your case the reader client didn't get it yet before reading and was still using the old metadata.
>
> And the cephfs itself won't guarantee that the reader clients could get latest contents and metadata immediately. There will be a delay between clients. It should the apps themself to guarantee this.
>
> Before commit c3d8e0b5de487a7c462781745bc17694a4266696 it won't check the local 'i_size' and just read contents beyond of the file end boundary from Rados. Even we allow it like this for the contents, but the metadatas still have the same issue. So this sounds incorrect to me.
>
> Greg, Venky, Patrick, Jeff
>
> What do you think ?
I'm a little worried about this phrasing. In a MIX mode, clients can
only have Frw caps (and the lack of Fs means, don't cache file size
locally); the MDS may broadcast file size updates but I don't think
clients are allowed to rely on that. So from looking at the userspace
client, what should be happening in the described scenario is, the
client extends the file size with the MDS, then does its
file-extending write to the OSD, then the write is reported as
complete. And the second client gets the read request, at which point
it would have to check the file size from the MDS, then see the larger
size, then do the OSD read.
...and actually, when the MDS gets a setattr for the size, it grabs an
xlock locally, which may mean it recalls the Frw caps while it makes
that change? I haven't had to dig into the guts of this in a while, so
maybe the local file size is reliable. I guess Fs also covers mtime,
and that's why we don't issue it in MIX mode? Ugh.
Anyway, it is definitely not up to the applications to try and provide
file consistency as that is why we have filesystems at all. :) We
definitely do guarantee that readers get contents and metadata which
reflect any completed writes, as of the time the read was initiated —
this is a basic requirement of POSIX. (Of course, if the write isn't
completed when the write starts, we can do whatever we want.)
I'm not generally up on the kernel implementation, so any patches
which may have broken it, and the correct fix, are beyond me.
But having said all that, layering samba on top of the kernel client
is going to have wildly unpredictable behavior — the kernel client can
maintain all the consistency it wants, but with Samba sitting on top
caching things then the real file state is going to be completely
uncontrolled. I don't see how you're going to get predictable
outcomes, and am surprised any kernel client cephfs change could
reliably influence this, although the referenced kernel patch
certainly does change what we return to reads when there is
simultaneous IO. What's the change from i_size_read(inode) -> i_size
do — is that correctly covered by cap state? Or is the problem here
that smbtorture is running on nodes going to multiple sambas that are
speaking to the kernel cephfs client and getting fairly random results
back due to that? (I could see eg smb1 does a truncate+write which
gets flushed to OSD, then smb2 does a read before the cephfs write is
finished which happens to pick up the new data while the size updates
are happening.)
-Greg
> I'm not sure if my understanding is correct. As a reference, here is my simple
> patch and I need more comments. The purpose of the patch is to prevent
> sync read handler from doing copy page when ret > i_size.
>
> Thanks.
>
>
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 220a41831b46..5897f52ee998 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -926,6 +926,9 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
>
> idx = 0;
> left = ret > 0 ? ret : 0;
> + if (left > i_size) {
> + left = i_size;
> + }
> while (left > 0) {
> size_t len, copied;
> page_off = off & ~PAGE_MASK;
> @@ -952,7 +955,7 @@ static ssize_t ceph_sync_read(struct kiocb *iocb, struct iov_iter *to,
> break;
> }
>
> - if (off > iocb->ki_pos) {
> + if (off > iocb->ki_pos || i_size == 0) {
>
> This seems incorrect to me.
>
> For example, what if the existing file size is 1024 bytes and then the writer client tries to append 1024 bytes to the file, then the file new size will be 2048. And then the reader clients' file i_size could still be 1024.
>
> Thanks
>
> - Xiubo
>
> if (off >= i_size) {
> *retry_op = CHECK_EOF;
> ret = i_size - iocb->ki_pos;
prev parent reply other threads:[~2024-02-21 17:12 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-24 3:25 Read operation gets EOF return when there is multi-client read/write after linux 5.16-rc1 Frank Hsiao 蕭法宣
2024-02-16 4:24 ` 回覆: " Frank Hsiao 蕭法宣
2024-02-19 2:08 ` Xiubo Li
2024-02-20 8:31 ` Xiubo Li
2024-02-21 1:41 ` Xiubo Li
[not found] ` <6f953a75-23cc-4d41-bbc5-2ca0a839f6d3@redhat.com>
2024-02-21 17:12 ` Gregory Farnum [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJ4mKGaSyNPMyCiM6WwyB9rAVGFr-_MYdwc5Q0k2YA4y0vOHWQ@mail.gmail.com \
--to=gfarnum@redhat.com \
--cc=ceph-devel@vger.kernel.org \
--cc=frankhsiao@qnap.com \
--cc=jlayton@kernel.org \
--cc=pdonnell@redhat.com \
--cc=vshankar@redhat.com \
--cc=xiubli@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).