Hi Ilya,
On Fri, 2025-01-17 at 13:06 +0100, Ilya Dryomov wrote:
> On Fri, Jan 17, 2025 at 4:51 AM Viacheslav Dubeyko <slava@xxxxxxxxxxx> wrote:
> >
> >
<skipped>
> > Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx>
> > ---
> > fs/ceph/addr.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 85936f6d2bf7..5e6ba92219f3 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -396,6 +396,15 @@ static void ceph_netfs_issue_read(struct netfs_io_subrequest *subreq)
> > struct page **pages;
> > size_t page_off;
> >
> > + /*
> > + * The io_iter.count needs to be corrected to aligned length.
> > + * Otherwise, iov_iter_get_pages_alloc2() operates with
> > + * the initial unaligned length value. As a result,
> > + * ceph_msg_data_cursor_init() triggers BUG_ON() in the case
> > + * if msg->sparse_read_total > msg->data_length.
> > + */
> > + subreq->io_iter.count = len;
>
> Hi Slava,
>
> So I take it that my hunch that it's subreq->io_iter and commenting out
> "len = err" assignment worked? TBH munging the count this way feels as
> much of an ugly workaround as ignoring iov_iter_get_pages_alloc2()
> return value (unless it's an error) to me.
>
As far as I can see, the main issue that subreq->io_iter keeps
unaligned value in subreq->io_iter.count. And if the alignment
was made earlier [1]:
len = subreq->len;
ceph_fscrypt_adjust_off_and_len(inode, &off, &len);
then it needs to correct the value of subreq->io_iter.count later
before calling iov_iter_get_pages_alloc2() [2]:
if (IS_ENCRYPTED(inode)) {
struct page **pages;
size_t page_off;
err = iov_iter_get_pages_alloc2(&subreq->io_iter, &pages, len, &page_off);
if (err < 0) {
doutc(cl, "%llx.%llx failed to allocate pages, %d\n",
ceph_vinop(inode), err);
goto out;
}
/* should always give us a page-aligned read */
WARN_ON_ONCE(page_off);
len = err;
err = 0;
osd_req_op_extent_osd_data_pages(req, 0, pages, len, 0, false,
false);
}
or ceph_netfs_issue_read() needs to be called with already
aligned value of subreq->io_iter.count.
If we have correct value (aligned) of subreq->io_iter.count, then
it doesn't matter "len = err" commented or not.
> Since this confirms that we have a regression introduced in ee4cdf7ba857
> ("netfs: Speed up buffered reading"), let's wait for David to chime in.
>
Sounds good!
Thanks,
Slava.
[1] https://elixir.bootlin.com/linux/v6.13-rc3/source/fs/ceph/addr.c#L365
[2] https://elixir.bootlin.com/linux/v6.13-rc3/source/fs/ceph/addr.c#L395
