Re: [RFC PATCH 1/8] fs: introduce get_shared_files() for dax&reflink

On Fri, Aug 07, 2020 at 09:13:29PM +0800, Shiyang Ruan wrote: Needs an xfs_ prefix... No error checking at all? What if rm_owner refers to metadata? When is the xfs_inode released? We don't iput it here, and there's no way for dax_unlock_page (afaict the only consumer) to do it, so we leak the reference. If there are millions of open files reflinked to this range of pmem this is going to allocate a /lot/ of memory. sfp->mapping = VFS_I(ip)->i_mapping; Why do we leave ->cookie uninitialized? What does it even do? Which device does this offset refer to? XFS supports multiple storage devices. Also, uh, is this really a pgoff_t? If yes, you can't use it with XFS_B_TO_FSB below without first converting it to a loff_t. No need to memset(0) rmap_low later, or zero rmap_high just to memset it later. "FSB" refers to xfs_fsblock_t. You just ripped the upper 32 bits off the fsblock number. Not necessary, bc_rec is zero in a freshly created cursor. Looking at this, I don't think this is the right way to approach memory poisoning. Rather than allocating a (potentially huge) linked list and passing it to the memory poison code to unmap pages, kill processes, and free the list, I think: 1) "->get_shared_files" should be more targetted. Call it ->storage_lost or something, so that it only has one purpose, which is to react to asynchronous notifications that storage has been lost. 2) The inner _get_shared_files_fn should directly call back into the memory manager to remove a poisoned page from the mapping and signal whatever process might have it mapped. That way, _get_shared_files_fn can look in the xfs buffer cache to see if we have a copy in DRAM, and immediately write it back to pmem. Hmm and now that you've gotten me rambling about hwpoison, I wonder what happens if dram backing part of the xfs buffer cache goes bad... --D > + > static const struct super_operations xfs_super_operations = { > .alloc_inode = xfs_fs_alloc_inode, > .destroy_inode = xfs_fs_destroy_inode, > @@ -1110,6 +1176,7 @@ static const struct super_operations xfs_super_operations = { > .show_options = xfs_fs_show_options, > .nr_cached_objects = xfs_fs_nr_cached_objects, > .free_cached_objects = xfs_fs_free_cached_objects, > + .get_shared_files = xfs_fs_get_shared_files, > }; > > static int > diff --git a/include/linux/dax.h b/include/linux/dax.h > index 6904d4e0b2e0..0a85e321d6b4 100644 > --- a/include/linux/dax.h > +++ b/include/linux/dax.h > @@ -40,6 +40,13 @@ struct dax_operations { > > extern struct attribute_group dax_attribute_group; > > +struct shared_files { > + struct list_head list; > + struct address_space *mapping; > + pgoff_t index; > + dax_entry_t cookie; > +}; > + > #if IS_ENABLED(CONFIG_DAX) > struct dax_device *dax_get_by_host(const char *host); > struct dax_device *alloc_dax(void *private, const char *host, > diff --git a/include/linux/fs.h b/include/linux/fs.h > index f5abba86107d..81de3d2739b9 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1977,6 +1977,8 @@ struct super_operations { > struct shrink_control *); > long (*free_cached_objects)(struct super_block *, > struct shrink_control *); > + int (*get_shared_files)(struct super_block *sb, pgoff_t offset, > + struct list_head *list); > }; > > /* > -- > 2.27.0 > > >