* [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
@ 2014-04-16 9:02 Qu Wenruo
2014-05-29 12:43 ` David Sterba
2014-05-30 7:51 ` Anand Jain
0 siblings, 2 replies; 7+ messages in thread
From: Qu Wenruo @ 2014-04-16 9:02 UTC (permalink / raw)
To: linux-btrfs; +Cc: Karel Zak
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-04-16 9:02 [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove Qu Wenruo
@ 2014-05-29 12:43 ` David Sterba
2014-05-30 1:27 ` Qu Wenruo
2014-06-04 5:51 ` Qu Wenruo
2014-05-30 7:51 ` Anand Jain
1 sibling, 2 replies; 7+ messages in thread
From: David Sterba @ 2014-05-29 12:43 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, Karel Zak
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)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-05-29 12:43 ` David Sterba
@ 2014-05-30 1:27 ` Qu Wenruo
2014-06-04 5:51 ` Qu Wenruo
1 sibling, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2014-05-30 1:27 UTC (permalink / raw)
To: dsterba, linux-btrfs, Karel Zak
-------- 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)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-04-16 9:02 [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove Qu Wenruo
2014-05-29 12:43 ` David Sterba
@ 2014-05-30 7:51 ` Anand Jain
2014-05-30 7:53 ` Anand Jain
2014-05-30 8:43 ` Qu Wenruo
1 sibling, 2 replies; 7+ messages in thread
From: Anand Jain @ 2014-05-30 7:51 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: Karel Zak
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:
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-05-30 7:51 ` Anand Jain
@ 2014-05-30 7:53 ` Anand Jain
2014-05-30 8:43 ` Qu Wenruo
1 sibling, 0 replies; 7+ messages in thread
From: Anand Jain @ 2014-05-30 7:53 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs; +Cc: Karel Zak
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-05-30 7:51 ` Anand Jain
2014-05-30 7:53 ` Anand Jain
@ 2014-05-30 8:43 ` Qu Wenruo
1 sibling, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2014-05-30 8:43 UTC (permalink / raw)
To: Anand Jain, linux-btrfs; +Cc: Karel Zak
-------- 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:
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove.
2014-05-29 12:43 ` David Sterba
2014-05-30 1:27 ` Qu Wenruo
@ 2014-06-04 5:51 ` Qu Wenruo
1 sibling, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2014-06-04 5:51 UTC (permalink / raw)
To: dsterba, linux-btrfs, Karel Zak
-------- 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)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-06-04 5:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-16 9:02 [PATCH RFC] btrfs: Add ctime/mtime update for btrfs device add/remove Qu Wenruo
2014-05-29 12:43 ` David Sterba
2014-05-30 1:27 ` Qu Wenruo
2014-06-04 5:51 ` Qu Wenruo
2014-05-30 7:51 ` Anand Jain
2014-05-30 7:53 ` Anand Jain
2014-05-30 8:43 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).