From: Fiona Ebner <f.ebner@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
Christian Ebner <c.ebner@proxmox.com>
Subject: Re: [pve-devel] [[PATCH kernel]] fix 5683: netfs: reset subreq iov iter before tail clean
Date: Tue, 22 Oct 2024 14:50:21 +0200 [thread overview]
Message-ID: <7d487881-6851-4c32-b2a2-dbb7ccdfe4e5@proxmox.com> (raw)
In-Reply-To: <20241002143624.1260363-1-c.ebner@proxmox.com>
Am 02.10.24 um 16:36 schrieb Christian Ebner:
> Fixes rare read corruption issues using the in kernel ceph client.
>
> On incomplete read requests, the clean tail flag should make sure to
> zero fill the remaining bytes for the subrequest.
> If the iov iterator is not at the correct position, this can however
> zero fill downloaded data, corrupting the read content.
>
> Link to issue:
> https://bugzilla.proxmox.com/show_bug.cgi?id=5683
>
> Link to upstream issue:
> https://bugzilla.kernel.org/show_bug.cgi?id=219237
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> This fixes the read corruption issue with my local reproducer.
>
> Providing a patched kernel to users affected by the issue for testing
> would be probably the best way to verify the fix.
>
> Also, I reached out once again to the kernel developers asking if
> this fix is a valid approach, hoping this can be included in current
> stable (as the patch does fix the issue also when applied on 6.11.1).
>
> ...et-subreq-iov-iter-before-tail-clean.patch | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
> create mode 100644 patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
>
> diff --git a/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch b/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
> new file mode 100644
> index 0000000..a87e722
> --- /dev/null
> +++ b/patches/kernel/0021-netfs-reset-subreq-iov-iter-before-tail-clean.patch
> @@ -0,0 +1,31 @@
> +From cd27abf0c555f39b12c05f9f6a8cb59ff25dfe45 Mon Sep 17 00:00:00 2001
> +From: Christian Ebner <c.ebner@proxmox.com>
> +Date: Wed, 2 Oct 2024 15:24:31 +0200
> +Subject: [PATCH] netfs: reset subreq iov iter before tail clean
> +
> +Make sure the iter is at the correct location when cleaning up tail
> +bytes for incomplete read subrequests.
> +
Disclaimer that I'm not familiar at all with the code.
So AFAIU, after short IO, the iov_iter_count() and subreq->len -
subreq->transferred might disagree. That is why before resubmission,
netfs_reset_subreq_iter() is called. That function aligns the iterator
position, so it will match the information from 'subreq'.
In your edge case, there is no resubmission though, because the
NETFS_SREQ_CLEAR_TAIL flag is set. But it still was short IO, so the
mentioned mismatch happened.
Now netfs_clear_unread() relies on the information from
iov_iter_count(), which does not match the actual 'subreq'. To fix it,
you call netfs_reset_subreq_iter() (like is done before resubmission) to
align that information.
Before commit 92b6cc5d1e7c ("netfs: Add iov_iters to (sub)requests to
describe various buffers"), the information from the 'subreq' was used
to set up the iterator:
> diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> index 7f753380e047..e9d408e211b8 100644
> --- a/fs/netfs/io.c
> +++ b/fs/netfs/io.c
> @@ -21,12 +21,7 @@
> */
> static void netfs_clear_unread(struct netfs_io_subrequest *subreq)
> {
> - struct iov_iter iter;
> -
> - iov_iter_xarray(&iter, ITER_DEST, &subreq->rreq->mapping->i_pages,
> - subreq->start + subreq->transferred,
> - subreq->len - subreq->transferred);
> - iov_iter_zero(iov_iter_count(&iter), &iter);
> + iov_iter_zero(iov_iter_count(&subreq->io_iter), &subreq->io_iter);
> }
so that sounds good :)
So with and without your change, after the netfs_clear_unread() call,
the iterator will be in the final position, i.e. iov_iter_count() == 0?
Then the information in 'subreq' is updated manually in the same branch
and it moves on to completion.
How far off from reality am I ;)? FWIW, the change looks okay to me, but
again, I'm not familiar with the code and I haven't done any testing
(and have no reproducer).
Of course it would be much nicer to have some confirmation from upstream
and/or users about this.
> +Fixes: 92b6cc5d ("netfs: Add iov_iters to (sub)requests to describe various buffers")
> +Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219237
> +
> +Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> +---
> + fs/netfs/io.c | 1 +
> + 1 file changed, 1 insertion(+)
> +
> +diff --git a/fs/netfs/io.c b/fs/netfs/io.c
> +index d6ada4eba744..500119285346 100644
> +--- a/fs/netfs/io.c
> ++++ b/fs/netfs/io.c
> +@@ -528,6 +528,7 @@ void netfs_subreq_terminated(struct netfs_io_subrequest *subreq,
> +
> + incomplete:
> + if (test_bit(NETFS_SREQ_CLEAR_TAIL, &subreq->flags)) {
> ++ netfs_reset_subreq_iter(rreq, subreq);
> + netfs_clear_unread(subreq);
> + subreq->transferred = subreq->len;
> + goto complete;
> +--
> +2.39.5
> +
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next prev parent reply other threads:[~2024-10-22 12:49 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-02 14:36 Christian Ebner
2024-10-22 12:50 ` Fiona Ebner [this message]
2024-10-22 16:06 ` Christian Ebner
2024-10-22 13:57 ` Thomas Lamprecht
2024-10-22 15:46 ` Christian Ebner
2024-10-23 8:28 ` Thomas Lamprecht
2024-10-23 8:51 ` Christian Ebner
2024-10-27 11:48 ` Christian Ebner
2024-10-23 13:52 ` [pve-devel] applied: " Thomas Lamprecht
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=7d487881-6851-4c32-b2a2-dbb7ccdfe4e5@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=c.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.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