Re: [PATCH v2 7/7] Use simple_start_creating() in various places.

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Sep 10, 2025 at 01:01:49PM +1000, NeilBrown wrote:
> On Tue, 09 Sep 2025, Al Viro wrote:
> > On Tue, Sep 09, 2025 at 02:43:21PM +1000, NeilBrown wrote:
> > 
> > >  	d_instantiate(dentry, inode);
> > > -	dget(dentry);
> > > -fail:
> > > -	inode_unlock(d_inode(parent));
> > > -	return dentry;
> > > +	return simple_end_creating(dentry);
> > 
> > No.  This is the wrong model - dget() belongs with d_instantiate()
> > here; your simple_end_creating() calling conventions are wrong.
> 
> I can see that I shouldn't have removed the dget() there - thanks.
> It is not entirely clear why hypfs_create_file() returns with two
> references held to the dentry....
> I see now one is added either to ->update_file or the list at
> hypfs_last_dentry, and the other is disposed of by kill_litter_super().
> 
> But apart from that one error is there something broader wrong with the
> patch?  You say "the wrong model" but I don't see it.

See below for hypfs:

commit c0c58d3cadfcefcf25f4885b47c47d6e6a3f9aee
Author: Al Viro <viro@zeniv.linux.org.uk>
Date:   Thu May 9 16:00:48 2024 -0400

    hypfs: don't pin dentries twice
    
    hypfs dentries end up with refcount 2 when they are not busy.
    Refcount 1 is enough to keep them pinned, and going that way
    allows to simplify things nicely:
            * don't need to drop an extra reference before the
    call of kill_litter_super() in ->kill_sb(); all we need
    there is to reset the cleanup list - everything on it will
    be taken out automatically.
            * we can make use of simple_recursive_removal() on
    tree rebuilds; just make sure that only children of root
    end up in the cleanup list and hypfs_delete_tree() becomes
    much simpler
    
    Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index 96409573c75d..a4dc8e13d999 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -61,33 +61,17 @@ static void hypfs_update_update(struct super_block *sb)
 
 static void hypfs_add_dentry(struct dentry *dentry)
 {
-	dentry->d_fsdata = hypfs_last_dentry;
-	hypfs_last_dentry = dentry;
-}
-
-static void hypfs_remove(struct dentry *dentry)
-{
-	struct dentry *parent;
-
-	parent = dentry->d_parent;
-	inode_lock(d_inode(parent));
-	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(d_inode(parent), dentry);
-		else
-			simple_unlink(d_inode(parent), dentry);
+	if (IS_ROOT(dentry->d_parent)) {
+		dentry->d_fsdata = hypfs_last_dentry;
+		hypfs_last_dentry = dentry;
 	}
-	d_drop(dentry);
-	dput(dentry);
-	inode_unlock(d_inode(parent));
 }
 
-static void hypfs_delete_tree(struct dentry *root)
+static void hypfs_delete_tree(void)
 {
 	while (hypfs_last_dentry) {
-		struct dentry *next_dentry;
-		next_dentry = hypfs_last_dentry->d_fsdata;
-		hypfs_remove(hypfs_last_dentry);
+		struct dentry *next_dentry = hypfs_last_dentry->d_fsdata;
+		simple_recursive_removal(hypfs_last_dentry, NULL);
 		hypfs_last_dentry = next_dentry;
 	}
 }
@@ -184,14 +168,14 @@ static ssize_t hypfs_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		rc = -EBUSY;
 		goto out;
 	}
-	hypfs_delete_tree(sb->s_root);
+	hypfs_delete_tree();
 	if (machine_is_vm())
 		rc = hypfs_vm_create_files(sb->s_root);
 	else
 		rc = hypfs_diag_create_files(sb->s_root);
 	if (rc) {
 		pr_err("Updating the hypfs tree failed\n");
-		hypfs_delete_tree(sb->s_root);
+		hypfs_delete_tree();
 		goto out;
 	}
 	hypfs_update_update(sb);
@@ -326,13 +310,9 @@ static void hypfs_kill_super(struct super_block *sb)
 {
 	struct hypfs_sb_info *sb_info = sb->s_fs_info;
 
-	if (sb->s_root)
-		hypfs_delete_tree(sb->s_root);
-	if (sb_info && sb_info->update_file)
-		hypfs_remove(sb_info->update_file);
-	kfree(sb->s_fs_info);
-	sb->s_fs_info = NULL;
+	hypfs_last_dentry = NULL;
 	kill_litter_super(sb);
+	kfree(sb_info);
 }
 
 static struct dentry *hypfs_create_file(struct dentry *parent, const char *name,
@@ -367,7 +347,6 @@ static struct dentry *hypfs_create_file(struct dentry *parent, const char *name,
 		BUG();
 	inode->i_private = data;
 	d_instantiate(dentry, inode);
-	dget(dentry);
 fail:
 	inode_unlock(d_inode(parent));
 	return dentry;