Additional Debug and other various pr_ additions

Additional Debug and other various pr_ additions

[Anthony Plack]

Would I step on anyone’s toes if I started submitting some extra patches to increase the verbosity of the BTRFS code in the kernel log?

I would probably start with most things as pr_debug just to keep it quiet on non-debug kernels, but I just thought that it might add a great deal of clarity to the code base and maybe help sysadmins figure out what is a BTRFS issue and what is some other issue.

I have read through the Developer, SubmittingPatches, and Coding Style.  I am at peace with git.

I am not promising everything done, but what I can help with doing, I will do.

As of right now there are:

36 	pr_ 
323 	WARN_ON
506 	BUG_ON

107,197 lines of code

Re: Additional Debug and other various pr_ additions

[David Sterba]

On Sun, May 24, 2015 at 09:42:19PM -0500, Anthony Plack wrote:
> Would I step on anyone’s toes if I started submitting some extra
> patches to increase the verbosity of the BTRFS code in the kernel log?
> 
> I would probably start with most things as pr_debug just to keep it
> quiet on non-debug kernels, but I just thought that it might add a
> great deal of clarity to the code base and maybe help sysadmins figure
> out what is a BTRFS issue and what is some other issue.

Starting with pr_debug should be safe. There may be different oppinions
where to put the messages as this can easily fall into "too trivial"
category. This probably needs some "taste" and past experience with
debugging problems to find the good candidate spots in the code. Please
send a sample and let's see.

[PATCH] Btrfs: Log the error conditions to determine which caused the error in btrfs_init_inode_security

[Anthony Plack]

> On May 25, 2015, at 7:57 AM, David Sterba <dsterba@suse.cz> wrote:
> 
> On Sun, May 24, 2015 at 09:42:19PM -0500, Anthony Plack wrote:
>> Would I step on anyone’s toes if I started submitting some extra
>> patches to increase the verbosity of the BTRFS code in the kernel log?
>> 
>> I would probably start with most things as pr_debug just to keep it
>> quiet on non-debug kernels, but I just thought that it might add a
>> great deal of clarity to the code base and maybe help sysadmins figure
>> out what is a BTRFS issue and what is some other issue.
> 
> Starting with pr_debug should be safe. There may be different oppinions
> where to put the messages as this can easily fall into "too trivial"
> category. This probably needs some "taste" and past experience with
> debugging problems to find the good candidate spots in the code. Please
> send a sample and let's see.

Good point and to clarify…

I am only looking at existing decision points in the code, where there is an error check already.  The goal is not to have debug data for the sake of debug data.  More simply, when an error appears and pr_debug is enabled, we at least log the error and the data involved.  This should provide us a nice chain of error issues in the kernel log before any WARN or BUG events.

I am climbing the mountain of struct right now, trying to get short, but meaningful data elements in the message.  These two can be a matter of taste and I am sure the learning will continue on my end.

This is an early change, and I will really appreciate comments on these to get these right.
</comment>


Log the error conditions to determine which caused the error in btrfs_init_inode_security


Signed-off-by: Anthony Plack <anthony@plack.net>
—
fs/btrfs/inode.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0c0bb45..c1a8884 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -117,6 +117,10 @@ static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
        err = btrfs_init_acl(trans, inode, dir);
        if (!err)
                err = btrfs_xattr_security_init(trans, inode, dir, qstr);
+               if (err)
+                       pr_debug("BTRFS: Unable to init xattr security. error=%d,transid=%d,inode=%d,dir=%d,qstr=%s", err, trans->transid, inode->i_uid, dir->i_uid, qstr->name);
+       else
+               pr_debug("BTRFS: Unable to init acl. error=%d,transid=%d,inode=%d,dir=%d,qstr=%s", err, trans->transid, inode->i_uid, dir->i_uid, qstr->name);
        return err;
 }

--
2.3.2

Re: [PATCH] Btrfs: Log the error conditions to determine which caused the error in btrfs_init_inode_security

[David Sterba]

On Mon, May 25, 2015 at 09:41:04AM -0500, Anthony Plack wrote:
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -117,6 +117,10 @@ static int btrfs_init_inode_security(struct btrfs_trans_handle *trans,
>         err = btrfs_init_acl(trans, inode, dir);
>         if (!err)
>                 err = btrfs_xattr_security_init(trans, inode, dir, qstr);

In this example, I think that the additional pr_debug does not help.
btrfs_xattr_security_init is a smple wrapper around the generic function
security_inode_init_security so basically all errors are not from btrfs
but the sercurity subsystem.

And this has either its own way to report problems or the error code is
returned back to userspace. In this case its create, mknod or mkdir (ie.
the callers of btrfs_xattr_security_init).

> +               if (err)
> +                       pr_debug("BTRFS: Unable to init xattr security. error=%d,transid=%d,inode=%d,dir=%d,qstr=%s", err, trans->transid, inode->i_uid, dir->i_uid, qstr->name);

- error is returned back to userspace,
- transid is not IMHO interesing as we're probably not going to use it
  to compare to any past or future transid
- uid/gid and name should be put into the security context, that's out
  of btrfs reach

I think it would be better to first start in functions that are part of
the user interface and somehow specific to btrfs (ie. the ioctls).

Another tip is to enhance debugging in some specific subsystem of btrfs.
The existing example is enospc. At the moment I don't have 100% sure
pointers so we might need a few more rounds.