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....
Cheers,
Dave.
--
Dave Chinner
david(a)fromorbit.com