Re: [RFC PATCH v2 0/6] fsdax: introduce fs query to support reflink

[Ruan Shiyang]

Hi Dave,

On 2020/11/30 上午6:47, Dave Chinner wrote:
> On Mon, Nov 23, 2020 at 08:41:10AM +0800, Shiyang Ruan wrote:
> > The call trace is like this:
> >   memory_failure()
> >     pgmap->ops->memory_failure()   => pmem_pgmap_memory_failure()
> >      gendisk->fops->block_lost()   => pmem_block_lost() or
> >                                           md_blk_block_lost()
> >       sb->s_ops->storage_lost()    => xfs_fs_storage_lost()
> >        xfs_rmap_query_range()
> >         xfs_storage_lost_helper()
> >          mf_recover_controller->recover_fn => \
> >                              memory_failure_dev_pagemap_kill_procs()
> >
> > The collect_procs() and kill_procs() are moved into a callback which
> > is passed from memory_failure() to xfs_storage_lost_helper().  So we
> > can call it when a file assocaited is found, instead of creating a
> > file list and iterate it.
> >
> > The fsdax & reflink support for XFS is not contained in this patchset.
>
> This looks promising - the overall architecture is a lot more
> generic and less dependent on knowing about memory, dax or memory
> failures. A few comments that I think would further improve
> understanding the patchset and the implementation:

Thanks for your kindly comment.  It gives me confidence.

> - the order of the patches is inverted. It should start with a
>    single patch introducing the mf_recover_controller structure for
>    callbacks, then introduce pgmap->ops->memory_failure, then
>    ->block_lost, then the pmem and md implementations of ->block
>    list, then ->storage_lost and the XFS implementations of
>    ->storage_lost.

Yes, it will be easier to understand the patchset in this order.

But I have something unsure: for example, I introduce ->memory_failure() 
firstly, but the implementation of ->memory_failure() needs to call 
->block_lost() which is supposed to be introduced in the next patch. So, 
I am not sure the code is supposed to be what in the implementation of 
->memory_failure() in pmem?  To avoid this situation, I committed the 
patches in the inverted order: lowest level first, then its caller, and 
then caller's caller.

I am trying to sort out the order.  How about this:
 Patch i.
   Introduce ->memory_failure()
      - just introduce interface, without implementation
 Patch i++.
   Introduce ->block_lost()
      - introduce interface and implement ->memory_failure()
         in pmem, so that it can call ->block_lost()
 Patch i++.
   (similar with above, skip...)

> - I think the names "block_lost" and "storage_lost" are misleading.
>    It's more like a "media failure" or a general "data corruption"
>    event at a specific physical location. The data may not be "lost"
>    but only damaged, so we might be able to recover from it without
>    "losing" anything. Hence I think they could be better named,
>    perhaps just "->corrupt_range"

'corrupt' sounds better.  (I'm not good at naming functions...)

> - need to pass a {offset,len} pair through the chain, not just a
>    single offset. This will allow other types of devices to report
>    different ranges of failures, from a single sector to an entire
>    device.

Yes, it's better to add the length.  I restrictively thought that 
memory-failure on pmem should affect one single page at one time.

> - I'm not sure that passing the mf_recover_controller structure
>    through the corruption event chain is the right thing to do here.
>    A block device could generate this storage failure callback if it
>    detects an unrecoverable error (e.g. during a MD media scrub or
>    rebuild/resilver failure) and in that case we don't have PFNs or
>    memory device failure functions to perform.
>
>    IOWs, I think the action that is taken needs to be independent of
>    the source that generated the error. Even for a pmem device, we
>    can be using the page cache, so it may be possible to recover the
>    pmem error by writing the cached page (if it exists) back over the
>    pmem.
>
>    Hence I think that the recover function probably needs to be moved
>    to the address space ops, because what we do to recover from the
>    error is going to be dependent on type of mapping the filesystem
>    is using. If it's a DAX mapping, we call back into a generic DAX
>    function that does the vma walk and process kill functions. If it
>    is a page cache mapping, then if the page is cached then we can
>    try to re-write it to disk to fix the bad data, otherwise we treat
>    it like a writeback error and report it on the next
>    write/fsync/close operation done on that file.
>
>    This gets rid of the mf_recover_controller altogether and allows
>    the interface to be used by any sort of block device for any sort
>    of bottom-up reporting of media/device failures.

Moving the recover function to the address_space ops looks a better 
idea. But I think that the error handler for page cache mapping is 
finished well in memory-failure.  The memory-failure is also reused to 
handles anonymous page.  If we move the recover function to 
address_space ops, I think we also need to refactor the existing handler 
for page cache mapping, which may affect anonymous page handling.  This 
makes me confused...


I rewrote the call trace:
memory_failure()
 * dax mapping case
 pgmap->ops->memory_failure()          =>
                                   pmem_pgmap_memory_failure()
  gendisk->fops->block_corrupt_range() =>
                                   - pmem_block_corrupt_range()
                                   - md_blk_block_corrupt_range()
   sb->s_ops->storage_currupt_range()  =>
                                   xfs_fs_storage_corrupt_range()
    xfs_rmap_query_range()
     xfs_storage_lost_helper()
      mapping->a_ops->corrupt_range()  =>
                                   xfs_dax_aops.xfs_dax_corrupt_range
       memory_failure_dev_pagemap_kill_procs()

 * page cache mapping case
 mapping->a_ops->corrupt_range()       =>
                                   xfs_address_space_operations.xfs_xxx
  memory_failure_generic_kill_procs()

It's rough and not completed yet.  Hope for your comment.

--
Thanks,
Ruan Shiyang.

> Cheers,
>
> Dave.