Re: [PATCH v2 22/50] convert efivarfs

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-10-28 at 00:45 +0000, Al Viro wrote:
> Initially filesystem is populated with d_alloc_name() + d_add().
> That becomes d_alloc_name() + d_make_persistent() + dput().
> Dynamic creation is switched to d_make_persistent();
> removal - to simple_unlink() (no point open-coding it in
> efivarfs_unlink(), better call it there)
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

The patch looks fine to me ... you can add an ack if you're collecting
them, but don't bother if you're not.

This caught my eye, though:

[...]
> @@ -278,7 +278,8 @@ static int efivarfs_create_dentry(struct
> super_block *sb, efi_char16_t *name16,
>  	inode->i_private = entry;
>  	i_size_write(inode, size + sizeof(__u32)); /* attributes +
> data */
>  	inode_unlock(inode);
> -	d_add(dentry, inode);
> +	d_make_persistent(dentry, inode);
> +	dput(dentry);

That dput looks misplaced in a creation routine and this is a common
pattern in pseudo filesystems that either pre-populate the dentry state
or create effectively unused dentries on other changes.  I know not
every pseudo filesystem does this, but it did make me wonder if it
should have it's own API, say d_create_persistent()?

Regards,

James