Re: [PATCH 02/11] iomap: copy the xfs writeback code to iomap.c

[Dave Chinner <david@fromorbit.com>]

On Tue, Oct 08, 2019 at 08:34:36AM +0200, Christoph Hellwig wrote:
> On Tue, Oct 08, 2019 at 08:43:53AM +1100, Dave Chinner wrote:
> > > +static int
> > > +iomap_ioend_compare(void *priv, struct list_head *a, struct list_head *b)
> > > +{
> > > +	struct iomap_ioend *ia, *ib;
> > > +
> > > +	ia = container_of(a, struct iomap_ioend, io_list);
> > > +	ib = container_of(b, struct iomap_ioend, io_list);
> > > +	if (ia->io_offset < ib->io_offset)
> > > +		return -1;
> > > +	else if (ia->io_offset > ib->io_offset)
> > > +		return 1;
> > > +	return 0;
> > 
> > No need for the else here.
> 
> That is usually my comment :)  But in this case it is just copied over
> code, so I didn't want to do cosmetic changes.

*nod*

> > > +	/*
> > > +	 * Given that we do not allow direct reclaim to call us, we should
> > > +	 * never be called while in a filesystem transaction.
> > > +	 */
> > > +	if (WARN_ON_ONCE(current->flags & PF_MEMALLOC_NOFS))
> > > +		goto redirty;
> > 
> > Is this true for all expected callers of these functions rather than
> > just XFS? i.e. PF_MEMALLOC_NOFS is used by transactions in XFS to
> > prevent transaction context recursion, but other filesystems do not
> > do this..
> > 
> > FWIW, I can also see that this is going to cause us problems if high
> > level code starts using memalloc_nofs_save() and then calling
> > filemap_datawrite() and friends...
> 
> We have the check for direct reclaim just above, so any file system
> using this iomap code will not allow direct reclaim.  Which I think is
> a very good idea given that direct reclaim through the file system is
> a very bad idea.

*nod*

> That leaves with only the filemap_datawrite case, which so far is
> theoretical.  If that ever becomes a think it is very obvious and we
> can just remove the debug check.

I expect it will be a thing sooner rather than later...

> > > +iomap_writepage(struct page *page, struct writeback_control *wbc,
> > > +		struct iomap_writepage_ctx *wpc,
> > > +		const struct iomap_writeback_ops *ops)
> > > +{
> > > +	int ret;
> > > +
> > > +	wpc->ops = ops;
> > > +	ret = iomap_do_writepage(page, wbc, wpc);
> > > +	if (!wpc->ioend)
> > > +		return ret;
> > > +	return iomap_submit_ioend(wpc, wpc->ioend, ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(iomap_writepage);
> > 
> > Can we kill ->writepage for iomap users, please? After all, we don't
> > mostly don't allow memory reclaim to do writeback of dirty pages,
> > and that's the only caller of ->writepage.
> 
> I'd rather not do this as part of this move.  But if you could expedite
> your patch to kill ->writepage from the large block size support patch
> and submit it ASAP on top of this series I would be very much in favor.

Ok, looks like the usual of more follow up patches on top of these.
I'm kinda waiting for these to land before porting the large block
size stuff on top of it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com