[Cluster-devel] [PATCH v4 06/11] iomap: Add write_{begin, end} iomap operations

[Andreas Grünbacher]

2018-05-18 18:04 GMT+02:00 Christoph Hellwig <hch at lst.de>:
> On Tue, May 15, 2018 at 10:16:52AM +0200, Andreas Gruenbacher wrote:
>> > Yes.  And from a quick look we can easily handle "stuffed" aka inline
>> > data with the existing architecture.  We just need a new IOMAP_INLINE
>> > flag for the extent type. Details will need to be worked out how to
>> > pass that data, either a struct page or virtual address should probably
>> > do it.
>>
>> This is about data journaling, it has nothing to do with "stuffed"
>> files per se. With journaled data writes, we need to do some special
>> processing for each page so that the page ends up in the journal
>> before it gets written back to its proper on-disk location. It just
>> happens that since we need these per-page operations anyway, the
>> "unstuffing" and "re-stuffing" nicely fits in those operations as
>> well.
>
> Your series makes two uses of the callbacks - one use case is to
> deal with inline data.

Yes, it's not perfect.

> The right way to deal with that is to make
> inline data an explicit iomap type (done in my next posting of the
> buffer head removal series), and then have iomap_begin find the data,
> kmap it and return it in the iomap, with iomap_end doing any fixups.

I see what you mean. How would iomap_write_actor deal with
IOMAP_INLINE -- just skip iomap_write_begin and iomap_write_end and
grab the page from the iomap? I'm a bit worried about reintroducing
the deadlock that iomap_write_actor so carefully tries to avoid.

> For the data journaing I agree that we need a post-write per page
> callout, but I think this should be just a simple optional callout
> just for data journaling, which doesn't override the remaining
> write_end handling.
>
>> Been there. An earlier version of the patches did have a separate
>> stuffed write path and the unstuffing and re-stuffing didn't happen in
>> those per-page operations. The result was much messier overall.
>
> Pointer, please.

It's in this patch:

  https://patchwork.kernel.org/patch/10191007/

from this posting:

https://www.spinics.net/lists/linux-fsdevel/msg121318.html

Thanks,
Andreas