From: Su Yue <l@damenly.org>
To: Brian Foster <bfoster@redhat.com>
Cc: Su Yue <l@damenly.org>,
Kent Overstreet <kent.overstreet@linux.dev>,
linux-bcachefs@vger.kernel.org
Subject: Re: [PATCH] bcachefs: fix iov_iter count underflow on sub-block dio read
Date: Thu, 15 Feb 2024 14:59:35 +0800 [thread overview]
Message-ID: <cysyb36w.fsf@damenly.org> (raw)
In-Reply-To: <Zc0C32sbI2zjSDgL@bfoster>
On Wed 14 Feb 2024 at 13:13, Brian Foster <bfoster@redhat.com>
wrote:
> On Fri, Feb 09, 2024 at 10:39:43AM +0800, Su Yue wrote:
>>
>> On Mon 05 Feb 2024 at 14:38, Brian Foster <bfoster@redhat.com>
>> wrote:
>>
>> > On Mon, Feb 05, 2024 at 02:15:36PM -0500, Kent Overstreet
>> > wrote:
>> > > On Mon, Feb 05, 2024 at 10:48:14AM -0500, Brian Foster
>> > > wrote:
>> > > > bch2_direct_IO_read() checks the request offset and size
>> > > > for >
>> > > sector
>> > > > alignment and then falls through to a couple calculations
>> > > > to >
>> > > shrink
>> > > > the size of the request based on the inode size. The
>> > > > problem > is
>> > > that
>> > > > these checks round up to the fs block size, which runs
>> > > > the > risk
>> > > of
>> > > > underflowing iter->count if the block size happens to be
>> > > > > large
>> > > > enough. This is triggered by fstest generic/361 with a 4k
>> > > > > block
>> > > > size, which subsequently leads to a crash.
>> > > >
>> > > > After some discussion, the original purpose of the
>> > > > shorten > logic
>> > > in this
>> > > > path is not totally clear. It appears to be intended as
>> > > > an >
>> > > optimization
>> > > > of limited value, so simplify things and avoid the
>> > > > underflow >
>> > > problem by
>> > > > removing it.
>> > >
>> > > Thinking about this a bit more...
>> > >
>> > > All that we're avoiding with the shorten is zeroing out the
>> > > rest of
>> > > the
>> > > read, which will happen in bch2_read_extent(); we won't be
>> > > issuing
>> > > any
>> > > IO, since that would require real data extents above
>> > > i_size, which
>> > > is of
>> > > course not allowed.
>> > >
>> > > But zeroing out the rest of the read is actual work that's
>> > > being
>> > > avoided, since if we return a short read (which is what
>> > > we're doing
>> > > here) it's undefined what happens to the rest of the
>> > > buffer.
>> > >
>> > > So... maybe we should keep it, for the crazy but inevitable
>> > > application
>> > > that uses a 1MB read buffer for everything, including
>> > > O_DIRECT reads
>> > > to
>> > > 4k files?
>> > >
>> >
>> > I had another change around that basically just protected
>> > against the
>> > underflow. I.e., something along the lines of the following,
>> > to also
>> > cover the additional use of shorten later in the function:
>> >
>> > if (shorten >= iter->count)
>> > shorten = 0;
>> >
Sorry for the misleading reply.
I mean the check of shorten is efficient and simpler which looks
good to me.
>>
>> The change is a clean fix without breaking logic in
>> bch2_direct_IO_read.
>> If without shroten, the next loop counting on iter->count must
>> be altered.
>> I'd like the simpler way.
>>
>
> Hi Su,
>
> I'm not quite following your comment on the "next loop counting
> on
> iter->count." Can you elaborate? Also, which option are you
> calling more
> simple? :) Thanks.
>
I think I just explained Kent's opinion in another way. The loop
is
fs/bcachefs/fs-io-direct.c:
static int bch2_direct_IO_read(struct kiocb *req, struct iov_iter
*iter) {
91 iter->count -= shorten;
...
130 while (iter->count) {
...
bch2_read()
}
iter->count += shorten;
}
bch2_direct_IO_read() does all boundary check for dio. If shorten
is
removed and no other condition in the loop, it brings more bios,
more btree searches in bch2_read[_extent].
--
Su
> Brian
>
>> --
>> Su
>>
>> > ... and IIRC that seemed to work as well. But I'll probably
>> > have to take
>> > a closer look at that large buffer case to grok what's
>> > happening and the
>> > original purpose of this logic.
>> >
>> > Brian
>>
prev parent reply other threads:[~2024-02-15 7:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-05 15:48 [PATCH] bcachefs: fix iov_iter count underflow on sub-block dio read Brian Foster
2024-02-05 19:15 ` Kent Overstreet
2024-02-05 19:38 ` Brian Foster
2024-02-09 2:39 ` Su Yue
2024-02-14 18:13 ` Brian Foster
2024-02-15 6:59 ` Su Yue [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=cysyb36w.fsf@damenly.org \
--to=l@damenly.org \
--cc=bfoster@redhat.com \
--cc=kent.overstreet@linux.dev \
--cc=linux-bcachefs@vger.kernel.org \
/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).