Re: [PATCH 1/4] btrfs: declare free_ipath() via DEFINE_FREE() instead

["Miquel Sabaté Solà" <mssola@mssola.com>]

[-- Attachment #1: Type: text/plain, Size: 6772 bytes --]

David Sterba @ 2025-10-22 09:31 +02:

> On Tue, Oct 21, 2025 at 04:27:46PM +0200, Miquel Sabaté Solà wrote:
>> This transforms the signature to __free_ipath() instead of the original
>> free_ipath(), but this function was only being used as a cleanup
>> function anyways. Hence, define it as a helper and use it via the
>> __free() attribute on all uses. This change also means that
>> __free_ipath() will be inlined whereas that wasn't the case for the
>> original one, but this shouldn't be a problem.
>>
>> A follow up macro like we do with BTRFS_PATH_AUTO_FREE() has been
>> discarded as the usage of this struct is not as widespread as that.
>
> The point of adding the macros or at least the freeing wrappers is to
> force the NULL initialization and to make it visible in the declarations
> (typed all in capitals). The number of use should not be the main factor
> and in this case it's 4 files.
>
>> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
>> ---
>>  fs/btrfs/backref.c | 10 +---------
>>  fs/btrfs/backref.h |  7 ++++++-
>>  fs/btrfs/inode.c   |  4 +---
>>  fs/btrfs/ioctl.c   |  3 +--
>>  fs/btrfs/scrub.c   |  4 +---
>>  5 files changed, 10 insertions(+), 18 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index e050d0938dc4..a1456402752a 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -2785,7 +2785,7 @@ struct btrfs_data_container *init_data_container(u32 total_bytes)
>>   * allocates space to return multiple file system paths for an inode.
>>   * total_bytes to allocate are passed, note that space usable for actual path
>>   * information will be total_bytes - sizeof(struct inode_fs_paths).
>> - * the returned pointer must be freed with free_ipath() in the end.
>> + * the returned pointer must be freed with __free_ipath() in the end.
>>   */
>>  struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
>>  					struct btrfs_path *path)
>> @@ -2810,14 +2810,6 @@ struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
>>  	return ifp;
>>  }
>>
>> -void free_ipath(struct inode_fs_paths *ipath)
>> -{
>> -	if (!ipath)
>> -		return;
>> -	kvfree(ipath->fspath);
>> -	kfree(ipath);
>> -}
>> -
>>  struct btrfs_backref_iter *btrfs_backref_iter_alloc(struct btrfs_fs_info *fs_info)
>>  {
>>  	struct btrfs_backref_iter *ret;
>> diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h
>> index 25d51c246070..d3b1ad281793 100644
>> --- a/fs/btrfs/backref.h
>> +++ b/fs/btrfs/backref.h
>> @@ -241,7 +241,12 @@ char *btrfs_ref_to_path(struct btrfs_root *fs_root, struct btrfs_path *path,
>>  struct btrfs_data_container *init_data_container(u32 total_bytes);
>>  struct inode_fs_paths *init_ipath(s32 total_bytes, struct btrfs_root *fs_root,
>>  					struct btrfs_path *path);
>> -void free_ipath(struct inode_fs_paths *ipath);
>> +
>> +DEFINE_FREE(ipath, struct inode_fs_paths *,
>> +	if (_T) {
>
> You can drop the if() as kvfree/kfree handles NULL pointers and we don't
> do that in the struct btrfs_path either.
>
>> +		kvfree(_T->fspath);
>> +		kfree(_T);
>> +	})
>>
>>  int btrfs_find_one_extref(struct btrfs_root *root, u64 inode_objectid,
>>  			  u64 start_off, struct btrfs_path *path,
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 79732756b87f..4d154209d70b 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -130,7 +130,7 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  	struct btrfs_fs_info *fs_info = warn->fs_info;
>>  	struct extent_buffer *eb;
>>  	struct btrfs_inode_item *inode_item;
>> -	struct inode_fs_paths *ipath = NULL;
>> +	struct inode_fs_paths *ipath __free(ipath) = NULL;
>
> I'd spell the name in full, like __free(free_ipath) or
> __free(inode_fs_paths) so it matches the type not the variable name.
>

The first option is not possible with __free as it would add "__free_"
automatically to the name. We'd need to go with __cleanup, and so it
would look like this:

     struct inode_fs_paths *ipath __cleanup(__free_ipath) = NULL;

Which is quite a mouthful :) So if you don't mind I'll go with the
second option you are suggesting for v2. Hence:

     struct inode_fs_paths *ipath __free(inode_fs_paths) = NULL;

>>  	struct btrfs_root *local_root;
>>  	struct btrfs_key key;
>>  	unsigned int nofs_flag;
>> @@ -193,7 +193,6 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  	}
>>
>>  	btrfs_put_root(local_root);
>> -	free_ipath(ipath);
>>  	return 0;
>>
>>  err:
>> @@ -201,7 +200,6 @@ static int data_reloc_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  "checksum error at logical %llu mirror %u root %llu inode %llu offset %llu, path resolving failed with ret=%d",
>>  		   warn->logical, warn->mirror_num, root, inum, offset, ret);
>>
>> -	free_ipath(ipath);
>>  	return ret;
>>  }
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 692016b2b600..453832ded917 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -3298,7 +3298,7 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
>>  	u64 rel_ptr;
>>  	int size;
>>  	struct btrfs_ioctl_ino_path_args *ipa = NULL;
>> -	struct inode_fs_paths *ipath = NULL;
>> +	struct inode_fs_paths *ipath __free(ipath) = NULL;
>>  	struct btrfs_path *path;
>>
>>  	if (!capable(CAP_DAC_READ_SEARCH))
>> @@ -3346,7 +3346,6 @@ static long btrfs_ioctl_ino_to_path(struct btrfs_root *root, void __user *arg)
>>
>>  out:
>>  	btrfs_free_path(path);
>> -	free_ipath(ipath);
>>  	kfree(ipa);
>>
>>  	return ret;
>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> index fe266785804e..74d8af1ff02d 100644
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -505,7 +505,7 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  	struct btrfs_inode_item *inode_item;
>>  	struct scrub_warning *swarn = warn_ctx;
>>  	struct btrfs_fs_info *fs_info = swarn->dev->fs_info;
>> -	struct inode_fs_paths *ipath = NULL;
>> +	struct inode_fs_paths *ipath __free(ipath) = NULL;
>>  	struct btrfs_root *local_root;
>>  	struct btrfs_key key;
>>
>> @@ -569,7 +569,6 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  				  (char *)(unsigned long)ipath->fspath->val[i]);
>>
>>  	btrfs_put_root(local_root);
>> -	free_ipath(ipath);
>>  	return 0;
>>
>>  err:
>> @@ -580,7 +579,6 @@ static int scrub_print_warning_inode(u64 inum, u64 offset, u64 num_bytes,
>>  			  swarn->physical,
>>  			  root, inum, offset, ret);
>>
>> -	free_ipath(ipath);
>>  	return 0;
>>  }
>>
>> --
>> 2.51.1
>>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 897 bytes --]