[PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Qu Wenruo]

Btrfs will send uevent to udev inform the device change,
but ctime/mtime for the block device inode is not udpated, which cause
libblkid used by btrfs-progs unable to detect device change and use old
cache, causing 'btrfs dev scan; btrfs dev rmove; btrfs dev scan' give an
error message.

Reported-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
Cc: Karel Zak <kzak@redhat.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 49d7fab..ce232d7 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1452,6 +1452,22 @@ out:
 	return ret;
 }
 
+/*
+ * Function to update ctime/mtime for a given device path.
+ * Mainly used for ctime/mtime based probe like libblkid.
+ */
+static void update_dev_time(char *path_name)
+{
+	struct file *filp;
+
+	filp = filp_open(path_name, O_RDWR, 0);
+	if (!filp)
+		return;
+	file_update_time(filp);
+	filp_close(filp, NULL);
+	return;
+}
+
 static int btrfs_rm_dev_item(struct btrfs_root *root,
 			     struct btrfs_device *device)
 {
@@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 	ret = 0;
 
-	/* Notify udev that device has changed */
-	if (bdev)
+	if (bdev) {
+		/* Notify udev that device has changed */
 		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
+		/* Update ctime/mtime for device path for libblkid */
+		update_dev_time(device_path);
+	}
+
 error_brelse:
 	brelse(bh);
 	if (bdev)
@@ -2146,6 +2166,8 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
 		ret = btrfs_commit_transaction(trans, root);
 	}
 
+	/* Update ctime/mtime for libblkid */
+	update_dev_time(device_path);
 	return ret;
 
 error_trans:
-- 
1.9.2

Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[David Sterba]

On Wed, Apr 16, 2014 at 05:02:32PM +0800, Qu Wenruo wrote:
> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  
>  	ret = 0;
>  
> -	/* Notify udev that device has changed */
> -	if (bdev)
> +	if (bdev) {
> +		/* Notify udev that device has changed */
>  		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>  
> +		/* Update ctime/mtime for device path for libblkid */
> +		update_dev_time(device_path);

The change on the device comes after the uevent notification, is it
possible that the event is delivered and processed before the device
times are updated? I would say so.

> +	}
> +
>  error_brelse:
>  	brelse(bh);
>  	if (bdev)

Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device 
add/remove.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年05月29日 20:43
> On Wed, Apr 16, 2014 at 05:02:32PM +0800, Qu Wenruo wrote:
>> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>>   
>>   	ret = 0;
>>   
>> -	/* Notify udev that device has changed */
>> -	if (bdev)
>> +	if (bdev) {
>> +		/* Notify udev that device has changed */
>>   		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>>   
>> +		/* Update ctime/mtime for device path for libblkid */
>> +		update_dev_time(device_path);
> The change on the device comes after the uevent notification, is it
> possible that the event is delivered and processed before the device
> times are updated? I would say so.
Yes, udev event is delivered before device times updated, but now 
btrfs-progs still use libblkid to do device scan things
as default, so I didn't catch the point.

Would you please tell me what the problem is?

Thanks,
Qu
>
>> +	}
>> +
>>   error_brelse:
>>   	brelse(bh);
>>   	if (bdev)

Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device 
add/remove.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年05月29日 20:43
> On Wed, Apr 16, 2014 at 05:02:32PM +0800, Qu Wenruo wrote:
>> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>>   
>>   	ret = 0;
>>   
>> -	/* Notify udev that device has changed */
>> -	if (bdev)
>> +	if (bdev) {
>> +		/* Notify udev that device has changed */
>>   		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>>   
>> +		/* Update ctime/mtime for device path for libblkid */
>> +		update_dev_time(device_path);
> The change on the device comes after the uevent notification, is it
> possible that the event is delivered and processed before the device
> times are updated? I would say so.
If I understand the udev thing right, uevent notification is sent to 
udevd and udevd process the notification.
For btrfs_rm_device(), it will send KOBJ_CHANGE uevent to udevd, and 
udevd will *update* the ctime/mtime in /dev.

But the problems is, there is a time lag between uevent sending and 
udevd updating the ctime/mtime,
which makes the ctime/mtime updating later than next 'btrfs dev scan' 
commands.
Since the ctime/mtime is not changed, libblkid used by 'btrfs dev scan' 
will still consider the removed disk as btrfs,
causing the bug I reported.

IMO the best method to deal the bug is to wait udevd processing the 
uevent, but unfortunately, we can't even guarantee
that there is a udevd processing the uevent(like some embedded system 
without udevd).

So I use update_dev_time() as a workaround, which provide a completely 
synchronize method to update device file ctime/mtime.
And even the uevent is processed before the update_dev_time(), it makes 
no harm except a new ctime/mtime.

Thanks,
Qu
>
>> +	}
>> +
>>   error_brelse:
>>   	brelse(bh);
>>   	if (bdev)

Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Anand Jain]

Hi Qu,

  In line below...

On 16/04/14 17:02, Qu Wenruo wrote:
> Btrfs will send uevent to udev inform the device change,
> but ctime/mtime for the block device inode is not udpated, which cause
> libblkid used by btrfs-progs unable to detect device change and use old
> cache, causing 'btrfs dev scan; btrfs dev rmove; btrfs dev scan' give an
> error message.
>
> Reported-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
> Cc: Karel Zak <kzak@redhat.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> ---
>   fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 49d7fab..ce232d7 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1452,6 +1452,22 @@ out:
>   	return ret;
>   }
>
> +/*
> + * Function to update ctime/mtime for a given device path.
> + * Mainly used for ctime/mtime based probe like libblkid.
> + */
> +static void update_dev_time(char *path_name)
> +{
> +	struct file *filp;
> +
> +	filp = filp_open(path_name, O_RDWR, 0);
> +	if (!filp)
> +		return;
> +	file_update_time(filp);
> +	filp_close(filp, NULL);
> +	return;
> +}
> +

   IMO /(I might be wrong) I think its not a good idea to explicitly
   achieve this. Since this thread would have already scratched the
   device's SB, shouldn't that take care of updating the mtime ?

Thanks, Anand


>   static int btrfs_rm_dev_item(struct btrfs_root *root,
>   			     struct btrfs_device *device)
>   {
> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>
>   	ret = 0;
>
> -	/* Notify udev that device has changed */
> -	if (bdev)
> +	if (bdev) {
> +		/* Notify udev that device has changed */
>   		btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>
> +		/* Update ctime/mtime for device path for libblkid */
> +		update_dev_time(device_path);
> +	}
> +



>   error_brelse:
>   	brelse(bh);
>   	if (bdev)
> @@ -2146,6 +2166,8 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path)
>   		ret = btrfs_commit_transaction(trans, root);
>   	}
>
> +	/* Update ctime/mtime for libblkid */
> +	update_dev_time(device_path);
>   	return ret;
>
>   error_trans:
>

Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Anand Jain]

I have sent this
  [PATCH] btrfs: kobject_uevent should use bd_part instead of bd_disk

which should fix. Could you try and update.

Thanks, Anand

On 30/05/14 15:51, Anand Jain wrote:
>
>
>
>
> Hi Qu,
>
>   In line below...
>
> On 16/04/14 17:02, Qu Wenruo wrote:
>> Btrfs will send uevent to udev inform the device change,
>> but ctime/mtime for the block device inode is not udpated, which cause
>> libblkid used by btrfs-progs unable to detect device change and use old
>> cache, causing 'btrfs dev scan; btrfs dev rmove; btrfs dev scan' give an
>> error message.
>>
>> Reported-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> Cc: Karel Zak <kzak@redhat.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>   fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 49d7fab..ce232d7 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1452,6 +1452,22 @@ out:
>>       return ret;
>>   }
>>
>> +/*
>> + * Function to update ctime/mtime for a given device path.
>> + * Mainly used for ctime/mtime based probe like libblkid.
>> + */
>> +static void update_dev_time(char *path_name)
>> +{
>> +    struct file *filp;
>> +
>> +    filp = filp_open(path_name, O_RDWR, 0);
>> +    if (!filp)
>> +        return;
>> +    file_update_time(filp);
>> +    filp_close(filp, NULL);
>> +    return;
>> +}
>> +
>
>    IMO /(I might be wrong) I think its not a good idea to explicitly
>    achieve this. Since this thread would have already scratched the
>    device's SB, shouldn't that take care of updating the mtime ?
>
> Thanks, Anand
>
>
>>   static int btrfs_rm_dev_item(struct btrfs_root *root,
>>                    struct btrfs_device *device)
>>   {
>> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root,
>> char *device_path)
>>
>>       ret = 0;
>>
>> -    /* Notify udev that device has changed */
>> -    if (bdev)
>> +    if (bdev) {
>> +        /* Notify udev that device has changed */
>>           btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>>
>> +        /* Update ctime/mtime for device path for libblkid */
>> +        update_dev_time(device_path);
>> +    }
>> +
>
>
>
>>   error_brelse:
>>       brelse(bh);
>>       if (bdev)
>> @@ -2146,6 +2166,8 @@ int btrfs_init_new_device(struct btrfs_root
>> *root, char *device_path)
>>           ret = btrfs_commit_transaction(trans, root);
>>       }
>>
>> +    /* Update ctime/mtime for libblkid */
>> +    update_dev_time(device_path);
>>       return ret;
>>
>>   error_trans:
>>
> --
> 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 RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device 
add/remove.
From: Anand Jain <Anand.Jain@oracle.com>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>, linux-btrfs@vger.kernel.org
Date: 2014年05月30日 15:51
>
>
>
>
> Hi Qu,
>
>  In line below...
>
> On 16/04/14 17:02, Qu Wenruo wrote:
>> Btrfs will send uevent to udev inform the device change,
>> but ctime/mtime for the block device inode is not udpated, which cause
>> libblkid used by btrfs-progs unable to detect device change and use old
>> cache, causing 'btrfs dev scan; btrfs dev rmove; btrfs dev scan' give an
>> error message.
>>
>> Reported-by: Tsutomu Itoh <t-itoh@jp.fujitsu.com>
>> Cc: Karel Zak <kzak@redhat.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>   fs/btrfs/volumes.c | 26 ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 49d7fab..ce232d7 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1452,6 +1452,22 @@ out:
>>       return ret;
>>   }
>>
>> +/*
>> + * Function to update ctime/mtime for a given device path.
>> + * Mainly used for ctime/mtime based probe like libblkid.
>> + */
>> +static void update_dev_time(char *path_name)
>> +{
>> +    struct file *filp;
>> +
>> +    filp = filp_open(path_name, O_RDWR, 0);
>> +    if (!filp)
>> +        return;
>> +    file_update_time(filp);
>> +    filp_close(filp, NULL);
>> +    return;
>> +}
>> +
>
>   IMO /(I might be wrong) I think its not a good idea to explicitly
>   achieve this. Since this thread would have already scratched the
>   device's SB, shouldn't that take care of updating the mtime ?
>
> Thanks, Anand
Yes, scratching SB will update the time of the device's inode.
But the problem is that, the *block file*'s ctime/mtime is not changed 
according to the device's inode mtime/ctime.

So in the patch, I use the device's *full path* not the *device's inode* 
to update ctime/mtime since kernel operations
will not update ctime/mtine of *device files under /dev*.

About your fix patch ([PATCH] btrfs: kobject_uevent should use bd_part 
instead of bd_disk )
I am unable to reproduce the problem on 3.15-rc4 kernel with 3.14.1 
btrfs-progs. :(

But the ctime/mtime things will not change as the following example:
------
# btrfs dev scan;stat /dev/sdd ; btrfs dev del /dev/sdd 
/mnt/scratch/;stat /dev/sdd ; btrfs dev scan
Scanning for Btrfs filesystems
   File: '/dev/sdd'
   Size: 0             Blocks: 0          IO Block: 4096   block special 
file
Device: 5h/5d    Inode: 8569        Links: 1     Device type: 8,30
Access: (0660/brw-rw----)  Uid: (    0/    root)   Gid: (    6/ disk)
Access: 2014-05-30 16:39:06.104006687 +0800
Modify: 2014-05-30 16:38:57.638006272 +0800 <<<ctime/mtime does not change
Change: 2014-05-30 16:38:57.638006272 +0800
  Birth: -
   File: '/dev/sdd'
   Size: 0             Blocks: 0          IO Block: 4096   block special 
file
Device: 5h/5d    Inode: 8569        Links: 1     Device type: 8,30
Access: (0660/brw-rw----)  Uid: (    0/    root)   Gid: (    6/ disk)
Access: 2014-05-30 16:39:06.104006687 +0800
Modify: 2014-05-30 16:38:57.638006272 +0800 <<<ctime/mtime does not change
Change: 2014-05-30 16:38:57.638006272 +0800
  Birth: -
Scanning for Btrfs filesystems
^^^ But no error message now.... :(
------

Thanks,
Qu
>
>
>>   static int btrfs_rm_dev_item(struct btrfs_root *root,
>>                    struct btrfs_device *device)
>>   {
>> @@ -1704,10 +1720,14 @@ int btrfs_rm_device(struct btrfs_root *root, 
>> char *device_path)
>>
>>       ret = 0;
>>
>> -    /* Notify udev that device has changed */
>> -    if (bdev)
>> +    if (bdev) {
>> +        /* Notify udev that device has changed */
>>           btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
>>
>> +        /* Update ctime/mtime for device path for libblkid */
>> +        update_dev_time(device_path);
>> +    }
>> +
>
>
>
>>   error_brelse:
>>       brelse(bh);
>>       if (bdev)
>> @@ -2146,6 +2166,8 @@ int btrfs_init_new_device(struct btrfs_root 
>> *root, char *device_path)
>>           ret = btrfs_commit_transaction(trans, root);
>>       }
>>
>> +    /* Update ctime/mtime for libblkid */
>> +    update_dev_time(device_path);
>>       return ret;
>>
>>   error_trans:
>>