[PATCH 1/2] btrfs: device add must be sysloged

[PATCH 1/2] btrfs: device add must be sysloged

[Anand Jain]

when we add a new disk to the mounted btrfs we don't record it
as of now, disk add is a critical change of btrfs configuration,
it must be recorded in the syslog to help offline investigations
of customer problems when reported.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 86d5474..16d20df 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2488,6 +2488,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	ret = btrfs_init_new_device(root, vol_args->name);
 
+	if (!ret)
+		btrfs_info(root->fs_info, "disk added %s",vol_args->name);
+
 	kfree(vol_args);
 out:
 	mutex_unlock(&root->fs_info->volume_mutex);
-- 
2.0.0.257.g75cc6c6

[PATCH 2/2] btrfs: device delete must be sysloged

[Anand Jain]

as in the disk add patch, disk detached from the volume must be
recorded in the syslog as well for the same reason.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
---
 fs/btrfs/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 16d20df..be4e097 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2530,6 +2530,9 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
 
+	if (!ret)
+		btrfs_info(root->fs_info, "disk deleted %s",vol_args->name);
+
 out:
 	kfree(vol_args);
 	mnt_drop_write_file(file);
-- 
2.0.0.257.g75cc6c6

Re: [PATCH 1/2] btrfs: device add must be sysloged

[David Sterba]

On Tue, Jul 01, 2014 at 12:58:56AM +0800, Anand Jain wrote:
> when we add a new disk to the mounted btrfs we don't record it
> as of now, disk add is a critical change of btrfs configuration,
> it must be recorded in the syslog to help offline investigations
> of customer problems when reported.
> 
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 86d5474..16d20df 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2488,6 +2488,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
>  	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>  	ret = btrfs_init_new_device(root, vol_args->name);
>  
> +	if (!ret)
> +		btrfs_info(root->fs_info, "disk added %s",vol_args->name);

Please use 'device' instead, I think we're moving away from 'disk'
everywhere else. Nitpick: space after the ,

Otherwise good,

Reviewed-by: David Sterba <dsterba@suse.cz>

Re: [PATCH 2/2] btrfs: device delete must be sysloged

[David Sterba]

On Tue, Jul 01, 2014 at 12:58:57AM +0800, Anand Jain wrote:
> as in the disk add patch, disk detached from the volume must be
> recorded in the syslog as well for the same reason.
> 
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> ---
>  fs/btrfs/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 16d20df..be4e097 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2530,6 +2530,9 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>  	mutex_unlock(&root->fs_info->volume_mutex);
>  	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
>  
> +	if (!ret)
> +		btrfs_info(root->fs_info, "disk deleted %s",vol_args->name);

Same comment as for the 'add' patch

Reviewed-by: David Sterba <dsterba@suse.cz>

[PATCH 1/2 v2] btrfs: device add must be sysloged

[Anand Jain]

when we add a new disk to the mounted btrfs we don't record it
as of now, disk add is a critical change of btrfs configuration,
it must be recorded in the syslog to help offline investigations
of customer problems when reported.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
---
 v2: Accepts David's review comment, thanks

 fs/btrfs/ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 86d5474..4e10259 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2488,6 +2488,10 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
 	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
 	ret = btrfs_init_new_device(root, vol_args->name);
 
+	if (!ret)
+		btrfs_info(root->fs_info, "device added %s",
+						vol_args->name);
+
 	kfree(vol_args);
 out:
 	mutex_unlock(&root->fs_info->volume_mutex);
-- 
2.0.0.257.g75cc6c6

[PATCH 2/2 v2] btrfs: device delete must be sysloged

[Anand Jain]

as in the disk add patch, disk detached from the volume must be
recorded in the syslog as well for the same reason.

Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
Reviewed-by: David Sterba <dsterba@suse.cz>
---
 v2: Accepts David's review comments, thanks

 fs/btrfs/ioctl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4e10259..016a5eb 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2531,6 +2531,10 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
 	mutex_unlock(&root->fs_info->volume_mutex);
 	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
 
+	if (!ret)
+		btrfs_info(root->fs_info, "device deleted %s",
+							vol_args->name);
+
 out:
 	kfree(vol_args);
 	mnt_drop_write_file(file);
-- 
2.0.0.257.g75cc6c6

Re: [PATCH 1/2] btrfs: device add must be sysloged

[Anand Jain]

On 02/07/2014 01:46, David Sterba wrote:
> On Tue, Jul 01, 2014 at 12:58:56AM +0800, Anand Jain wrote:
>> when we add a new disk to the mounted btrfs we don't record it
>> as of now, disk add is a critical change of btrfs configuration,
>> it must be recorded in the syslog to help offline investigations
>> of customer problems when reported.
>>
>> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
>> ---
>>   fs/btrfs/ioctl.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 86d5474..16d20df 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2488,6 +2488,9 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
>>   	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>>   	ret = btrfs_init_new_device(root, vol_args->name);
>>
>> +	if (!ret)
>> +		btrfs_info(root->fs_info, "disk added %s",vol_args->name);
>
> Please use 'device' instead, I think we're moving away from 'disk'
> everywhere else. Nitpick: space after the ,

yes, you are right. Thanks for commenting.

> Otherwise good,
>
> Reviewed-by: David Sterba <dsterba@suse.cz>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2 v2] btrfs: device add must be sysloged

[Satoru Takeuchi]

(2014/07/02 10:37), Anand Jain wrote:
> when we add a new disk to the mounted btrfs we don't record it
> as of now, disk add is a critical change of btrfs configuration,
> it must be recorded in the syslog to help offline investigations
> of customer problems when reported.
> 
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> Reviewed-by: David Sterba <dsterba@suse.cz>

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

test result:
  3.16-rc3: failed
  3.16-rc3 with this patch: success

===============================================================================
#!/bin/sh

TEST_DEV1=/dev/vdb
TEST_DEV2=/dev/vdc
TEST_MNT=/home/sat/mnt

umount $TEST_MNT
mkfs.btrfs -f $TEST_DEV1
dmesg >dmesg.old

mount $TEST_DEV1 $TEST_MNT
btrfs dev add -f $TEST_DEV2 $TEST_MNT
if [ $? -ne 0 ] ; then
    echo "[ERROR] failed to btrfs dev add." >&2
    exit 1
fi

btrfs dev del $TEST_DEV2 $TEST_MNT
if [ $? -ne 0 ] ; then
    echo "[ERROR] failed to btrfs dev delete" >&2
    exit 1
fi

dmesg >dmesg.new
diff dmesg.old dmesg.new | grep '^>' >dmesg.diff

RET=0
TEST_DEV1_BASE=$(basename $TEST_DEV1)
if grep -q "BTRFS info (device $TEST_DEV1_BASE): device added $TEST_DEV2" dmesg.diff ; then
    echo "[PASS] btrfs device add is logged." >&2
else
    echo "[FAIL] btrfs device add is not logged." >&2
    RET=1
fi
    
if grep -q "BTRFS info (device $TEST_DEV1_BASE): device deleted $TEST_DEV2" dmesg.diff ; then
    echo "[PASS] btrfs device delete is logged." >&2
else
    echo "[FAIL] btrfs device delete is not logged." >&2
    RET=1
fi

rm dmesg.{new,old,diff}
umount $TEST_MNT

exit $RET
===============================================================================

Thanks,
Satoru

> ---
>   v2: Accepts David's review comment, thanks
> 
>   fs/btrfs/ioctl.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 86d5474..4e10259 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2488,6 +2488,10 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
>   	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>   	ret = btrfs_init_new_device(root, vol_args->name);
>   
> +	if (!ret)
> +		btrfs_info(root->fs_info, "device added %s",
> +						vol_args->name);
> +
>   	kfree(vol_args);
>   out:
>   	mutex_unlock(&root->fs_info->volume_mutex);
> 

Re: [PATCH 2/2 v2] btrfs: device delete must be sysloged

[Satoru Takeuchi]

(2014/07/02 10:37), Anand Jain wrote:
> as in the disk add patch, disk detached from the volume must be
> recorded in the syslog as well for the same reason.
> 
> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
> Reviewed-by: David Sterba <dsterba@suse.cz>

Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>

test result:
  3.16-rc3: failed
  3.16-rc3 with these patches: success

===============================================================================
#!/bin/sh

TEST_DEV1=/dev/vdb
TEST_DEV2=/dev/vdc
TEST_MNT=/home/sat/mnt

umount $TEST_MNT
mkfs.btrfs -f $TEST_DEV1
dmesg >dmesg.old

mount $TEST_DEV1 $TEST_MNT
btrfs dev add -f $TEST_DEV2 $TEST_MNT
if [ $? -ne 0 ] ; then
    echo "[ERROR] failed to btrfs dev add." >&2
    exit 1
fi

btrfs dev del $TEST_DEV2 $TEST_MNT
if [ $? -ne 0 ] ; then
    echo "[ERROR] failed to btrfs dev delete" >&2
    exit 1
fi

dmesg >dmesg.new
diff dmesg.old dmesg.new | grep '^>' >dmesg.diff

RET=0
TEST_DEV1_BASE=$(basename $TEST_DEV1)
if grep -q "BTRFS info (device $TEST_DEV1_BASE): device added $TEST_DEV2" dmesg.diff ; then
    echo "[PASS] btrfs device add is logged." >&2
else
    echo "[FAIL] btrfs device add is not logged." >&2
    RET=1
fi
    
if grep -q "BTRFS info (device $TEST_DEV1_BASE): device deleted $TEST_DEV2" dmesg.diff ; then
    echo "[PASS] btrfs device delete is logged." >&2
else
    echo "[FAIL] btrfs device delete is not logged." >&2
    RET=1
fi

rm dmesg.{new,old,diff}
umount $TEST_MNT

exit $RET
===============================================================================

Thanks,
Satoru



> ---
>   v2: Accepts David's review comments, thanks
> 
>   fs/btrfs/ioctl.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 4e10259..016a5eb 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2531,6 +2531,10 @@ static long btrfs_ioctl_rm_dev(struct file *file, void __user *arg)
>   	mutex_unlock(&root->fs_info->volume_mutex);
>   	atomic_set(&root->fs_info->mutually_exclusive_operation_running, 0);
>   
> +	if (!ret)
> +		btrfs_info(root->fs_info, "device deleted %s",
> +							vol_args->name);
> +
>   out:
>   	kfree(vol_args);
>   	mnt_drop_write_file(file);
> 

Re: [PATCH 1/2 v2] btrfs: device add must be sysloged

[Satoru Takeuchi]

(2014/07/02 16:02), Satoru Takeuchi wrote:
> (2014/07/02 10:37), Anand Jain wrote:
>> when we add a new disk to the mounted btrfs we don't record it
>> as of now, disk add is a critical change of btrfs configuration,
>> it must be recorded in the syslog to help offline investigations
>> of customer problems when reported.
>>
>> Signed-off-by: Anand Jain <Anand.Jain@oracle.com>
>> Reviewed-by: David Sterba <dsterba@suse.cz>
> 
> Reviewed-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> Tested-by: Satoru Takeuchi <takeuchi_satoru@jp.fujitsu.com>
> 
> test result:
>    3.16-rc3: failed
>    3.16-rc3 with this patch: success

Please let me correct.

> 
> ===============================================================================
> #!/bin/sh
> 
> TEST_DEV1=/dev/vdb
> TEST_DEV2=/dev/vdc
> TEST_MNT=/home/sat/mnt
> 
> umount $TEST_MNT
> mkfs.btrfs -f $TEST_DEV1
> dmesg >dmesg.old
> 
> mount $TEST_DEV1 $TEST_MNT
> btrfs dev add -f $TEST_DEV2 $TEST_MNT
> if [ $? -ne 0 ] ; then
>      echo "[ERROR] failed to btrfs dev add." >&2
>      exit 1
> fi
> 
> btrfs dev del $TEST_DEV2 $TEST_MNT
> if [ $? -ne 0 ] ; then
>      echo "[ERROR] failed to btrfs dev delete" >&2
>      exit 1
> fi
> 
> dmesg >dmesg.new
> diff dmesg.old dmesg.new | grep '^>' >dmesg.diff
> 
> RET=0
> TEST_DEV1_BASE=$(basename $TEST_DEV1)
> if grep -q "BTRFS info (device $TEST_DEV1_BASE): device added $TEST_DEV2" dmesg.diff ; then
>      echo "[PASS] btrfs device add is logged." >&2
> else
>      echo "[FAIL] btrfs device add is not logged." >&2
>      RET=1
> fi

[PASS] here,

>      
> if grep -q "BTRFS info (device $TEST_DEV1_BASE): device deleted $TEST_DEV2" dmesg.diff ; then
>      echo "[PASS] btrfs device delete is logged." >&2
> else
>      echo "[FAIL] btrfs device delete is not logged." >&2
>      RET=1
> fi

And [FAIL] here since 2/2 is not applied yet at this test.

Thanks,
Satoru

> 
> rm dmesg.{new,old,diff}
> umount $TEST_MNT
> 
> exit $RET
> ===============================================================================
> 
> Thanks,
> Satoru
> 
>> ---
>>    v2: Accepts David's review comment, thanks
>>
>>    fs/btrfs/ioctl.c | 4 ++++
>>    1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 86d5474..4e10259 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2488,6 +2488,10 @@ static long btrfs_ioctl_add_dev(struct btrfs_root *root, void __user *arg)
>>    	vol_args->name[BTRFS_PATH_NAME_MAX] = '\0';
>>    	ret = btrfs_init_new_device(root, vol_args->name);
>>    
>> +	if (!ret)
>> +		btrfs_info(root->fs_info, "device added %s",
>> +						vol_args->name);
>> +
>>    	kfree(vol_args);
>>    out:
>>    	mutex_unlock(&root->fs_info->volume_mutex);
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>