On Wed 14 Feb 2024 at 13:13, Brian Foster <[email protected]>
wrote:
On Fri, Feb 09, 2024 at 10:39:43AM +0800, Su Yue wrote:
On Mon 05 Feb 2024 at 14:38, Brian Foster <[email protected]>
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
