Re: [Question] About XFS random buffer write performance

[Zhengyuan Liu <liuzhengyuang521@gmail.com>]

Thanks for your discussions.
For this issue,  if we have plans to fix?

On Sat, Aug 1, 2020 at 6:13 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Fri, Jul 31, 2020 at 09:47:13PM +0100, Matthew Wilcox wrote:
> > On Fri, Jul 31, 2020 at 12:45:17AM +0100, Matthew Wilcox wrote:
> > > On Fri, Jul 31, 2020 at 08:08:57AM +1000, Dave Chinner wrote:
> > > > On Thu, Jul 30, 2020 at 02:50:40PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Jul 30, 2020 at 09:05:03AM +1000, Dave Chinner wrote:
> > > > > > On Wed, Jul 29, 2020 at 07:50:35PM +0100, Matthew Wilcox wrote:
> > > > > > > I had a bit of a misunderstanding.  Let's discard that proposal
> > > > > > > and discuss what we want to optimise for, ignoring THPs.  We don't
> > > > > > > need to track any per-block state, of course.  We could implement
> > > > > > > __iomap_write_begin() by reading in the entire page (skipping the last
> > > > > > > few blocks if they lie outside i_size, of course) and then marking the
> > > > > > > entire page Uptodate.
> > > > > >
> > > > > > __iomap_write_begin() already does read-around for sub-page writes.
> > > > > > And, if necessary, it does zeroing of unwritten extents, newly
> > > > > > allocated ranges and ranges beyond EOF and marks them uptodate
> > > > > > appropriately.
> > > > >
> > > > > But it doesn't read in the entire page, just the blocks in the page which
> > > > > will be touched by the write.
> > > >
> > > > Ah, you are right, I got my page/offset macros mixed up.
> > > >
> > > > In which case, you just identified why the uptodate array is
> > > > necessary and can't be removed. If we do a sub-page write() the page
> > > > is not fully initialised, and so if we then mmap it readpage needs
> > > > to know what part of the page requires initialisation to bring the
> > > > page uptodate before it is exposed to userspace.
> > >
> > > You snipped the part of my mail where I explained how we could handle
> > > that without the uptodate array ;-(  Essentially, we do as you thought
> > > it worked, we read the entire page (or at least the portion of it that
> > > isn't going to be overwritten.  Once all the bytes have been transferred,
> > > we can mark the page Uptodate.  We'll need to wait for the transfer to
> > > happen if the write overlaps a block boundary, but we do that right now.
> >
> > OK, so this turns out to be Hard.  We enter the iomap code with
> >
> > iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *iter,
> >                 const struct iomap_ops *ops)
> >
> > which does:
> >                 ret = iomap_apply(inode, pos, iov_iter_count(iter),
> >                                 IOMAP_WRITE, ops, iter, iomap_write_actor);
> >
> > so iomap_write_actor doesn't get told about the blocks in the page before
> > the starting pos.  They might be a hole or mapped; we have no idea.
>
> So this is a kind of the same problem block size > page size has to
> deal with for block allocation - the zero-around issue. THat is,
> when a sub block write triggers a new allocation, it actually has to
> zero the entire block in the page cache first, which means it needs
> to expand the IO range in iomap_write_actor()....
>
> https://lore.kernel.org/linux-xfs/20181107063127.3902-10-david@fromorbit.com/
> https://lore.kernel.org/linux-xfs/20181107063127.3902-14-david@fromorbit.com/
>
> > We could allocate pages _here_ and call iomap_readpage() for the pages
> > which overlap the beginning and end of the I/O,
>
> FWIW, this is effective what calling iomap_zero() from
> iomap_write_actor() does - it allocates pages outside the write
> range via iomap_begin_write(), then zeroes them in memory and marks
> them dirty....
>
> > but I'm not entirely
> > convinced that the iomap_ops being passed in will appreciate being
> > called for a read that has no intent to write the portions of the page
> > outside pos.
>
> I don't think it should matter what the range of the read being done
> is - it has the same constraints whether it's to populate the
> partial block or whole blocks just before the write. Especially as
> we are in the buffered write path and so the filesystem has
> guaranteed us exclusive access to the inode and it's mapping
> here....
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com