On 5/29/19 10:47 AM, Dave Chinner wrote:
On Wed, May 29, 2019 at 10:01:58AM +0800, Shiyang Ruan wrote:
On 5/28/19 5:17 PM, Jan Kara wrote:
On Mon 27-05-19 16:25:41, Shiyang Ruan wrote:
On 5/23/19 7:51 PM, Goldwyn Rodrigues wrote:
Hi,
I'm working on reflink & dax in XFS, here are some thoughts on this:
As mentioned above: the second iomap's offset and length must match the
first. I thought so at the beginning, but later found that the only
difference between these two iomaps is @addr. So, what about adding a
@saddr, which means the source address of COW extent, into the struct iomap.
The ->iomap_begin() fills @saddr if the extent is COW, and 0 if not. Then
handle this @saddr in each ->actor(). No more modifications in other
functions.
Yes, I started of with the exact idea before being recommended this by Dave.
I used two fields instead of one namely cow_pos and cow_addr which defined
the source details. I had put it as a iomap flag as opposed to a type
which of course did not appeal well.
We may want to use iomaps for cases where two inodes are involved.
An example of the other scenario where offset may be different is file
comparison for dedup: vfs_dedup_file_range_compare(). However, it would
need two inodes in iomap as well.
Yes, it is reasonable. Thanks for your explanation.
One more thing RFC:
I'd like to add an end-io callback argument in ->dax_iomap_actor() to update
the metadata after one whole COW operation is completed. The end-io can
also be called in ->iomap_end(). But one COW operation may call
->iomap_apply() many times, and so does the end-io. Thus, I think it would
be nice to move it to the bottom of ->dax_iomap_actor(), called just once in
each COW operation.
I'm sorry but I don't follow what you suggest. One COW operation is a call
to dax_iomap_rw(), isn't it? That may call iomap_apply() several times,
each invocation calls ->iomap_begin(), ->actor() (dax_iomap_actor()),
->iomap_end() once. So I don't see a difference between doing something in
->actor() and ->iomap_end() (besides the passed arguments but that does not
seem to be your concern). So what do you exactly want to do?
Hi Jan,
Thanks for pointing out, and I'm sorry for my mistake. It's
->dax_iomap_rw(), not ->dax_iomap_actor().
I want to call the callback function at the end of ->dax_iomap_rw().
Like this:
dax_iomap_rw(..., callback) {
...
while (...) {
iomap_apply(...);
}
if (callback != null) {
callback();
}
return ...;
}
Why does this need to be in dax_iomap_rw()?
We already do post-dax_iomap_rw() "io-end callbacks" directly in
xfs_file_dax_write() to update the file size....
Yes, but we also need to call ->xfs_reflink_end_cow() after a COW
operation. And an is-cow flag(from iomap) is also needed to determine
if we call it. I think it would be better to put this into
->dax_iomap_rw() as a callback function.
So sorry for my poor expression.
Cheers,
Dave.
--
Thanks,
Shiyang Ruan.
