Messages in this thread | | | Date | Fri, 8 Apr 2022 14:04:58 +0800 | Subject | Re: [PATCH v11 7/8] xfs: Implement ->notify_failure() for XFS | From | Shiyang Ruan <> |
| |
在 2022/3/30 14:00, Christoph Hellwig 写道: >> @@ -1892,6 +1893,8 @@ xfs_free_buftarg( >> list_lru_destroy(&btp->bt_lru); >> >> blkdev_issue_flush(btp->bt_bdev); >> + if (btp->bt_daxdev) >> + dax_unregister_holder(btp->bt_daxdev, btp->bt_mount); >> fs_put_dax(btp->bt_daxdev); >> >> kmem_free(btp); >> @@ -1939,6 +1942,7 @@ xfs_alloc_buftarg( >> struct block_device *bdev) >> { >> xfs_buftarg_t *btp; >> + int error; >> >> btp = kmem_zalloc(sizeof(*btp), KM_NOFS); >> >> @@ -1946,6 +1950,14 @@ xfs_alloc_buftarg( >> btp->bt_dev = bdev->bd_dev; >> btp->bt_bdev = bdev; >> btp->bt_daxdev = fs_dax_get_by_bdev(bdev, &btp->bt_dax_part_off); >> + if (btp->bt_daxdev) { >> + error = dax_register_holder(btp->bt_daxdev, mp, >> + &xfs_dax_holder_operations); >> + if (error) { >> + xfs_err(mp, "DAX device already in use?!"); >> + goto error_free; >> + } >> + } > > It seems to me that just passing the holder and holder ops to > fs_dax_get_by_bdev and the holder to dax_unregister_holder would > significantly simply the interface here. > > Dan, what do you think?
Hi Dan,
Could you give some advise on this API? Is it needed to move dax_register_holder's job into fs_dax_get_by_bdev()?
-- Thanks, Ruan
> >> +#if IS_ENABLED(CONFIG_MEMORY_FAILURE) && IS_ENABLED(CONFIG_FS_DAX) > > No real need for the IS_ENABLED. Also any reason to even build this > file if the options are not set? It seems like > xfs_dax_holder_operations should just be defined to NULL and the > whole file not supported if we can't support the functionality. > > Dan: not for this series, but is there any reason not to require > MEMORY_FAILURE for DAX to start with? > >> + >> + ddev_start = mp->m_ddev_targp->bt_dax_part_off; >> + ddev_end = ddev_start + >> + (mp->m_ddev_targp->bt_bdev->bd_nr_sectors << SECTOR_SHIFT) - 1; > > This should use bdev_nr_bytes. > > But didn't we say we don't want to support notifications on partitioned > devices and thus don't actually need all this? > >> + >> + /* Ignore the range out of filesystem area */ >> + if ((offset + len) < ddev_start) > > No need for the inner braces. > >> + if ((offset + len) > ddev_end) > > No need for the braces either. > >> diff --git a/fs/xfs/xfs_notify_failure.h b/fs/xfs/xfs_notify_failure.h >> new file mode 100644 >> index 000000000000..76187b9620f9 >> --- /dev/null >> +++ b/fs/xfs/xfs_notify_failure.h >> @@ -0,0 +1,10 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2022 Fujitsu. All Rights Reserved. >> + */ >> +#ifndef __XFS_NOTIFY_FAILURE_H__ >> +#define __XFS_NOTIFY_FAILURE_H__ >> + >> +extern const struct dax_holder_operations xfs_dax_holder_operations; >> + >> +#endif /* __XFS_NOTIFY_FAILURE_H__ */ > > Dowe really need a new header for this vs just sequeezing it into > xfs_super.h or something like that? > >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c >> index e8f37bdc8354..b8de6ed2c888 100644 >> --- a/fs/xfs/xfs_super.c >> +++ b/fs/xfs/xfs_super.c >> @@ -353,6 +353,12 @@ xfs_setup_dax_always( >> return -EINVAL; >> } >> >> + if (xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) { >> + xfs_alert(mp, >> + "need rmapbt when both DAX and reflink enabled."); >> + return -EINVAL; >> + } > > Right now we can't even enable reflink with DAX yet, so adding this > here seems premature - it should go into the patch allowing DAX+reflink. >
| |