Re: [PATCH v12 6/7] xfs: Implement ->notify_failure() for XFS

2022-04-12 Thread Dan Williams
On Tue, Apr 12, 2022 at 5:04 PM Dave Chinner  wrote:
>
> On Mon, Apr 11, 2022 at 12:09:03AM +0800, Shiyang Ruan wrote:
> > Introduce xfs_notify_failure.c to handle failure related works, such as
> > implement ->notify_failure(), register/unregister dax holder in xfs, and
> > so on.
> >
> > If the rmap feature of XFS enabled, we can query it to find files and
> > metadata which are associated with the corrupt data.  For now all we do
> > is kill processes with that file mapped into their address spaces, but
> > future patches could actually do something about corrupt metadata.
> >
> > After that, the memory failure needs to notify the processes who are
> > using those files.
> >
> > Signed-off-by: Shiyang Ruan 
> > ---
> >  fs/xfs/Makefile |   5 +
> >  fs/xfs/xfs_buf.c|   7 +-
> >  fs/xfs/xfs_fsops.c  |   3 +
> >  fs/xfs/xfs_mount.h  |   1 +
> >  fs/xfs/xfs_notify_failure.c | 219 
> >  fs/xfs/xfs_super.h  |   1 +
> >  6 files changed, 233 insertions(+), 3 deletions(-)
> >  create mode 100644 fs/xfs/xfs_notify_failure.c
> >
> > diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> > index 04611a1068b4..09f5560e29f2 100644
> > --- a/fs/xfs/Makefile
> > +++ b/fs/xfs/Makefile
> > @@ -128,6 +128,11 @@ xfs-$(CONFIG_SYSCTL) += xfs_sysctl.o
> >  xfs-$(CONFIG_COMPAT) += xfs_ioctl32.o
> >  xfs-$(CONFIG_EXPORTFS_BLOCK_OPS) += xfs_pnfs.o
> >
> > +# notify failure
> > +ifeq ($(CONFIG_MEMORY_FAILURE),y)
> > +xfs-$(CONFIG_FS_DAX) += xfs_notify_failure.o
> > +endif
> > +
> >  # online scrub/repair
> >  ifeq ($(CONFIG_XFS_ONLINE_SCRUB),y)
> >
> > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> > index f9ca08398d32..9064b8dfbc66 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -5,6 +5,7 @@
> >   */
> >  #include "xfs.h"
> >  #include 
> > +#include 
> >
> >  #include "xfs_shared.h"
> >  #include "xfs_format.h"
> > @@ -1911,7 +1912,7 @@ xfs_free_buftarg(
> >   list_lru_destroy(>bt_lru);
> >
> >   blkdev_issue_flush(btp->bt_bdev);
> > - fs_put_dax(btp->bt_daxdev, NULL);
> > + fs_put_dax(btp->bt_daxdev, btp->bt_mount);
> >
> >   kmem_free(btp);
> >  }
> > @@ -1964,8 +1965,8 @@ xfs_alloc_buftarg(
> >   btp->bt_mount = mp;
> >   btp->bt_dev =  bdev->bd_dev;
> >   btp->bt_bdev = bdev;
> > - btp->bt_daxdev = fs_dax_get_by_bdev(bdev, >bt_dax_part_off, NULL,
> > - NULL);
> > + btp->bt_daxdev = fs_dax_get_by_bdev(bdev, >bt_dax_part_off, mp,
> > + _dax_holder_operations);
>
> I see a problem with this: we are setting up notify callbacks before
> we've even read in the superblock during mount. i.e. we don't even
> kow yet if we've got an XFS filesystem on this block device.
>
> Hence if we get a notification immediately after registering this
> notification callback
>
> [...]
>
> > +
> > +static int
> > +xfs_dax_notify_ddev_failure(
> > + struct xfs_mount*mp,
> > + xfs_daddr_t daddr,
> > + xfs_daddr_t bblen,
> > + int mf_flags)
> > +{
> > + struct xfs_trans*tp = NULL;
> > + struct xfs_btree_cur*cur = NULL;
> > + struct xfs_buf  *agf_bp = NULL;
> > + int error = 0;
> > + xfs_fsblock_t   fsbno = XFS_DADDR_TO_FSB(mp, daddr);
> > + xfs_agnumber_t  agno = XFS_FSB_TO_AGNO(mp, fsbno);
> > + xfs_fsblock_t   end_fsbno = XFS_DADDR_TO_FSB(mp, daddr + 
> > bblen);
> > + xfs_agnumber_t  end_agno = XFS_FSB_TO_AGNO(mp, end_fsbno);
>
>  none of this code is going to function correctly because it
> is dependent on the superblock having been read, validated and
> copied to the in-memory superblock.
>
> > + error = xfs_trans_alloc_empty(mp, );
> > + if (error)
> > + return error;
>
> ... and it's not valid to use transactions (even empty ones) before
> log recovery has completed and set the log up correctly.
>
> > +
> > + for (; agno <= end_agno; agno++) {
> > + struct xfs_rmap_irecri_low = { };
> > + struct xfs_rmap_irecri_high;
> > + struct failure_info notify;
> > + struct xfs_agf  *agf;
> > + xfs_agblock_t   agend;
> > +
> > + error = xfs_alloc_read_agf(mp, tp, agno, 0, _bp);
> > + if (error)
> > + break;
> > +
> > + cur = xfs_rmapbt_init_cursor(mp, tp, agf_bp, agf_bp->b_pag);
>
> ... and none of the structures this rmapbt walk is dependent on
> (e.g. perag structures) have been initialised yet so there's null
> pointer dereferences going to happen here.
>
> Perhaps even worse is that the rmapbt is not guaranteed to be in
> consistent state until after log recovery has completed, so this
> walk could get stuck forever in a stale on-disk cycle that
> recovery 

Re: [PATCH v12 6/7] xfs: Implement ->notify_failure() for XFS

2022-04-12 Thread Dave Chinner
On Mon, Apr 11, 2022 at 12:09:03AM +0800, Shiyang Ruan wrote:
> Introduce xfs_notify_failure.c to handle failure related works, such as
> implement ->notify_failure(), register/unregister dax holder in xfs, and
> so on.
> 
> If the rmap feature of XFS enabled, we can query it to find files and
> metadata which are associated with the corrupt data.  For now all we do
> is kill processes with that file mapped into their address spaces, but
> future patches could actually do something about corrupt metadata.
> 
> After that, the memory failure needs to notify the processes who are
> using those files.
> 
> Signed-off-by: Shiyang Ruan 
> ---
>  fs/xfs/Makefile |   5 +
>  fs/xfs/xfs_buf.c|   7 +-
>  fs/xfs/xfs_fsops.c  |   3 +
>  fs/xfs/xfs_mount.h  |   1 +
>  fs/xfs/xfs_notify_failure.c | 219 
>  fs/xfs/xfs_super.h  |   1 +
>  6 files changed, 233 insertions(+), 3 deletions(-)
>  create mode 100644 fs/xfs/xfs_notify_failure.c
> 
> diff --git a/fs/xfs/Makefile b/fs/xfs/Makefile
> index 04611a1068b4..09f5560e29f2 100644
> --- a/fs/xfs/Makefile
> +++ b/fs/xfs/Makefile
> @@ -128,6 +128,11 @@ xfs-$(CONFIG_SYSCTL) += xfs_sysctl.o
>  xfs-$(CONFIG_COMPAT) += xfs_ioctl32.o
>  xfs-$(CONFIG_EXPORTFS_BLOCK_OPS) += xfs_pnfs.o
>  
> +# notify failure
> +ifeq ($(CONFIG_MEMORY_FAILURE),y)
> +xfs-$(CONFIG_FS_DAX) += xfs_notify_failure.o
> +endif
> +
>  # online scrub/repair
>  ifeq ($(CONFIG_XFS_ONLINE_SCRUB),y)
>  
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index f9ca08398d32..9064b8dfbc66 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -5,6 +5,7 @@
>   */
>  #include "xfs.h"
>  #include 
> +#include 
>  
>  #include "xfs_shared.h"
>  #include "xfs_format.h"
> @@ -1911,7 +1912,7 @@ xfs_free_buftarg(
>   list_lru_destroy(>bt_lru);
>  
>   blkdev_issue_flush(btp->bt_bdev);
> - fs_put_dax(btp->bt_daxdev, NULL);
> + fs_put_dax(btp->bt_daxdev, btp->bt_mount);
>  
>   kmem_free(btp);
>  }
> @@ -1964,8 +1965,8 @@ xfs_alloc_buftarg(
>   btp->bt_mount = mp;
>   btp->bt_dev =  bdev->bd_dev;
>   btp->bt_bdev = bdev;
> - btp->bt_daxdev = fs_dax_get_by_bdev(bdev, >bt_dax_part_off, NULL,
> - NULL);
> + btp->bt_daxdev = fs_dax_get_by_bdev(bdev, >bt_dax_part_off, mp,
> + _dax_holder_operations);

I see a problem with this: we are setting up notify callbacks before
we've even read in the superblock during mount. i.e. we don't even
kow yet if we've got an XFS filesystem on this block device.

Hence if we get a notification immediately after registering this
notification callback

[...]

> +
> +static int
> +xfs_dax_notify_ddev_failure(
> + struct xfs_mount*mp,
> + xfs_daddr_t daddr,
> + xfs_daddr_t bblen,
> + int mf_flags)
> +{
> + struct xfs_trans*tp = NULL;
> + struct xfs_btree_cur*cur = NULL;
> + struct xfs_buf  *agf_bp = NULL;
> + int error = 0;
> + xfs_fsblock_t   fsbno = XFS_DADDR_TO_FSB(mp, daddr);
> + xfs_agnumber_t  agno = XFS_FSB_TO_AGNO(mp, fsbno);
> + xfs_fsblock_t   end_fsbno = XFS_DADDR_TO_FSB(mp, daddr + bblen);
> + xfs_agnumber_t  end_agno = XFS_FSB_TO_AGNO(mp, end_fsbno);

 none of this code is going to function correctly because it
is dependent on the superblock having been read, validated and
copied to the in-memory superblock.

> + error = xfs_trans_alloc_empty(mp, );
> + if (error)
> + return error;

... and it's not valid to use transactions (even empty ones) before
log recovery has completed and set the log up correctly.

> +
> + for (; agno <= end_agno; agno++) {
> + struct xfs_rmap_irecri_low = { };
> + struct xfs_rmap_irecri_high;
> + struct failure_info notify;
> + struct xfs_agf  *agf;
> + xfs_agblock_t   agend;
> +
> + error = xfs_alloc_read_agf(mp, tp, agno, 0, _bp);
> + if (error)
> + break;
> +
> + cur = xfs_rmapbt_init_cursor(mp, tp, agf_bp, agf_bp->b_pag);

... and none of the structures this rmapbt walk is dependent on
(e.g. perag structures) have been initialised yet so there's null
pointer dereferences going to happen here.

Perhaps even worse is that the rmapbt is not guaranteed to be in
consistent state until after log recovery has completed, so this
walk could get stuck forever in a stale on-disk cycle that
recovery would have corrected

Hence these notifications need to be delayed until after the
filesystem is mounted, all the internal structures have been set up
and log recovery has completed.

Cheers,

Dave.
-- 
Dave Chinner
da...@fromorbit.com



Re: [PATCH v12 1/7] dax: Introduce holder for dax_device

2022-04-12 Thread Dan Williams
On Sun, Apr 10, 2022 at 9:09 AM Shiyang Ruan  wrote:
>
> To easily track filesystem from a pmem device, we introduce a holder for
> dax_device structure, and also its operation.  This holder is used to
> remember who is using this dax_device:
>  - When it is the backend of a filesystem, the holder will be the
>instance of this filesystem.
>  - When this pmem device is one of the targets in a mapped device, the
>holder will be this mapped device.  In this case, the mapped device
>has its own dax_device and it will follow the first rule.  So that we
>can finally track to the filesystem we needed.
>
> The holder and holder_ops will be set when filesystem is being mounted,
> or an target device is being activated.
>

Looks good to me:

Reviewed-by: Dan Williams 

I am assuming this will be staged in a common DAX branch, but holler
now if this should go through the XFS tree.