[PATCH] debufgs: debugfs_create_blob can set the file size

[PATCH] debufgs: debugfs_create_blob can set the file size

[Timur Tabi]

debugfs_create_blob() is given the size of the blob, so use it to
also set the size of the dentry.  For example, efi=debug previously
showed

-r-------- 1 root root 0 Feb  7 13:30 boot_services_code0
-r-------- 1 root root 0 Feb  7 13:30 boot_services_code1
-r-------- 1 root root 0 Feb  7 13:30 boot_services_data0
-r-------- 1 root root 0 Feb  7 13:30 boot_services_data1
-r-------- 1 root root 0 Feb  7 13:30 boot_services_data2
-r-------- 1 root root 0 Feb  7 13:30 boot_services_data3
-r-------- 1 root root 0 Feb  7 13:30 boot_services_data4

but with this patch it shows

-r-------- 1 root root  12783616 Feb  7 13:26 boot_services_code0
-r-------- 1 root root    262144 Feb  7 13:26 boot_services_code1
-r-------- 1 root root  41705472 Feb  7 13:26 boot_services_data0
-r-------- 1 root root  23187456 Feb  7 13:26 boot_services_data1
-r-------- 1 root root 110645248 Feb  7 13:26 boot_services_data2
-r-------- 1 root root   1048576 Feb  7 13:26 boot_services_data3
-r-------- 1 root root      4096 Feb  7 13:26 boot_services_data4

Signed-off-by: Timur Tabi <ttabi@nvidia.com>
---
 fs/debugfs/file.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index c6f4a9a98b85..d97800603a8f 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -1152,7 +1152,14 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
 				   struct dentry *parent,
 				   struct debugfs_blob_wrapper *blob)
 {
-	return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
+	struct dentry *dentry;
+
+	dentry = debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
+	if (!IS_ERR(dentry))
+		d_inode(dentry)->i_size = blob->size;
+
+	return dentry;
+
 }
 EXPORT_SYMBOL_GPL(debugfs_create_blob);
 
-- 
2.34.1

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Dave Chinner]

On Wed, Feb 07, 2024 at 02:06:19PM -0600, Timur Tabi wrote:
> debugfs_create_blob() is given the size of the blob, so use it to
> also set the size of the dentry.  For example, efi=debug previously
> showed
> 
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_code0
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_code1
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data0
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data1
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data2
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data3
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data4
> 
> but with this patch it shows
> 
> -r-------- 1 root root  12783616 Feb  7 13:26 boot_services_code0
> -r-------- 1 root root    262144 Feb  7 13:26 boot_services_code1
> -r-------- 1 root root  41705472 Feb  7 13:26 boot_services_data0
> -r-------- 1 root root  23187456 Feb  7 13:26 boot_services_data1
> -r-------- 1 root root 110645248 Feb  7 13:26 boot_services_data2
> -r-------- 1 root root   1048576 Feb  7 13:26 boot_services_data3
> -r-------- 1 root root      4096 Feb  7 13:26 boot_services_data4
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  fs/debugfs/file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index c6f4a9a98b85..d97800603a8f 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -1152,7 +1152,14 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>  				   struct dentry *parent,
>  				   struct debugfs_blob_wrapper *blob)
>  {
> -	return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
> +	struct dentry *dentry;
> +
> +	dentry = debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
> +	if (!IS_ERR(dentry))
> +		d_inode(dentry)->i_size = blob->size;

i_size_write()?

And why doesn't debugfs_create_file_unsafe() do this already when it
attaches the blob to the inode? 

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Timur Tabi]

On Thu, 2024-02-08 at 08:40 +1100, Dave Chinner wrote:
> i_size_write()?

Thanks, I will post a v2.

> 
> And why doesn't debugfs_create_file_unsafe() do this already when it
> attaches the blob to the inode? 

Because it doesn't know the size any more.

The 'blob' in debugfs_create_blob() is this:

struct debugfs_blob_wrapper {
	void *data;
	unsigned long size;
};

When it passes the blob to debugfs_create_file_unsafe(), that's passed as
"void *data", and so debugfs_create_file_unsafe() doesn't know that it's a
'blob' any more.

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Dave Chinner]

On Wed, Feb 07, 2024 at 09:44:19PM +0000, Timur Tabi wrote:
> On Thu, 2024-02-08 at 08:40 +1100, Dave Chinner wrote:
> > i_size_write()?
> 
> Thanks, I will post a v2.
> 
> > 
> > And why doesn't debugfs_create_file_unsafe() do this already when it
> > attaches the blob to the inode? 
> 
> Because it doesn't know the size any more.
> 
> The 'blob' in debugfs_create_blob() is this:
> 
> struct debugfs_blob_wrapper {
> 	void *data;
> 	unsigned long size;
> };
> 
> When it passes the blob to debugfs_create_file_unsafe(), that's passed as
> "void *data", and so debugfs_create_file_unsafe() doesn't know that it's a
> 'blob' any more.

So fix the debugfs_create_file_*() functions to pass a length
and that way you fix all the "debugfs files have zero length but
still have data that can be read" problems for everyone? Then the
zero length problem can be isolated to just the debug objects that don't
know their size (i.e. are streams of data, not fixed size).

IMO, it doesn't help anyone to have one part of the debugfs
blob/file API to set inode->i_size correctly, but then leave the
majority of the users still behaving the problematic way (i.e. zero
size yet with data that can be read). It's just a recipe for
confusion....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Darrick J. Wong]

On Wed, Feb 07, 2024 at 02:06:19PM -0600, Timur Tabi wrote:
> debugfs_create_blob() is given the size of the blob, so use it to
> also set the size of the dentry.  For example, efi=debug previously
> showed
> 
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_code0
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_code1
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data0
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data1
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data2
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data3
> -r-------- 1 root root 0 Feb  7 13:30 boot_services_data4
> 
> but with this patch it shows
> 
> -r-------- 1 root root  12783616 Feb  7 13:26 boot_services_code0
> -r-------- 1 root root    262144 Feb  7 13:26 boot_services_code1
> -r-------- 1 root root  41705472 Feb  7 13:26 boot_services_data0
> -r-------- 1 root root  23187456 Feb  7 13:26 boot_services_data1
> -r-------- 1 root root 110645248 Feb  7 13:26 boot_services_data2
> -r-------- 1 root root   1048576 Feb  7 13:26 boot_services_data3
> -r-------- 1 root root      4096 Feb  7 13:26 boot_services_data4
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  fs/debugfs/file.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index c6f4a9a98b85..d97800603a8f 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -1152,7 +1152,14 @@ struct dentry *debugfs_create_blob(const char *name, umode_t mode,
>  				   struct dentry *parent,
>  				   struct debugfs_blob_wrapper *blob)
>  {
> -	return debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
> +	struct dentry *dentry;
> +
> +	dentry = debugfs_create_file_unsafe(name, mode & 0644, parent, blob, &fops_blob);
> +	if (!IS_ERR(dentry))
> +		d_inode(dentry)->i_size = blob->size;

Aren't you supposed to use i_size_write for this?

--D

> +
> +	return dentry;
> +
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_blob);
>  
> -- 
> 2.34.1
> 
> 

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Timur Tabi]

On Thu, 2024-02-08 at 09:06 +1100, Dave Chinner wrote:
> So fix the debugfs_create_file_*() functions to pass a length
> and that way you fix all the "debugfs files have zero length but
> still have data that can be read" problems for everyone? Then the
> zero length problem can be isolated to just the debug objects that don't
> know their size (i.e. are streams of data, not fixed size).
> 
> IMO, it doesn't help anyone to have one part of the debugfs
> blob/file API to set inode->i_size correctly, but then leave the
> majority of the users still behaving the problematic way (i.e. zero
> size yet with data that can be read). It's just a recipe for
> confusion....

I don't disagree, but that's a much more ambitious change than I am prepared
to make.  

debugfs_create_file_size() already exists, it's just that
debugfs_create_blob() doesn't use it.  I think the problem is that the file
size is not always known, or at least not always the fixed, so setting the
initial file size isn't always an option.

On a side note, debugfs_create_file_size() does the same thing that my v1
patch does:

struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
if (!IS_ERR(de))
	d_inode(de)->i_size = file_size;

Shouldn't this function also use i_size_write()?

Re: [PATCH] debufgs: debugfs_create_blob can set the file size

[Dave Chinner]

On Wed, Feb 07, 2024 at 10:18:26PM +0000, Timur Tabi wrote:
> On Thu, 2024-02-08 at 09:06 +1100, Dave Chinner wrote:
> > So fix the debugfs_create_file_*() functions to pass a length
> > and that way you fix all the "debugfs files have zero length but
> > still have data that can be read" problems for everyone? Then the
> > zero length problem can be isolated to just the debug objects that don't
> > know their size (i.e. are streams of data, not fixed size).
> > 
> > IMO, it doesn't help anyone to have one part of the debugfs
> > blob/file API to set inode->i_size correctly, but then leave the
> > majority of the users still behaving the problematic way (i.e. zero
> > size yet with data that can be read). It's just a recipe for
> > confusion....
> 
> I don't disagree, but that's a much more ambitious change than I am prepared
> to make.  

TANSTAAFL.

"That's somebody else's problem" is what everyone says about fixing
problems once they've worked out the minimal fix that addresses
their specific issue.

We need a new rule: do not leave technical debt behind that other
people will still have to clean up after you get what you need.

> debugfs_create_file_size() already exists, it's just that
> debugfs_create_blob() doesn't use it.  I think the problem is that the file
> size is not always known, or at least not always the fixed, so setting the
> initial file size isn't always an option.

Yes, that's exactly what I said:

"Then the zero length problem can be isolated to just the debug
objects that don't know their size (i.e. are streams of data, not
fixed size)."

Make the majority behave correctly by default, force those that want
a file to work like a stream to explicitly say they want a zero
sized debugfs file.

Indeed - those that want a stream should call a helper function like
debugfs_create_data_stream() to indicate what is emitted from that
file is an unknown amount of data, and that it should be read until
no more data is present by userspace.

The hack of using zero length files for these can then be hidden
behind the API that documents exactly the type of data being
exported by the subsystem....

> On a side note, debugfs_create_file_size() does the same thing that my v1
> patch does:
> 
> struct dentry *de = debugfs_create_file(name, mode, parent, data, fops);
> if (!IS_ERR(de))
> 	d_inode(de)->i_size = file_size;
> 
> Shouldn't this function also use i_size_write()?

Yes, it should.

-Dave.
-- 
Dave Chinner
david@fromorbit.com