disk doesn't spin down with thin pool + dmeventd

RFC In review Alan Jenkins 2 participants 5 messages Jan 7, 2016 – Jan 11, 2016 Device mapper

Share & tools

Alan Jenkins

author

2016-01-07 19:31 UTC #0 lore raw

RFC 1/1 disk doesn't spin down with thin pool + dmeventd

I tried using Docker on my Fedora NAS box. It created a thin pool LV, which caused hard drive activity every ~10 seconds.

dmeventd queries the thin pool every 10 seconds, and it causes a transaction commit in order to make sure the statistics are up to date. But transactions are already supposed to be committed after 1 second. (See Documentation/device-mapper/thin-provisioning.txt, "Updating on-disk metadata").

It seems like a simple case of "don't do that". The kernel already lets us avoid the commit. How about it (patch below)? If it seems reasonable, I can whip up a commit message for it.

Thanks
Alan

daemons/dmeventd/dmeventd.c +9 -0

index c093d91..5948f15 100644
@@ -412,6 +412,15 @@ static struct thread_status *_alloc_thread_status(const struct message_data *dat
 	if (!(thread->device.name = dm_strdup(data->device_uuid)))
 		goto_out;
 
+	/*
+	 * Avoid repeatedly forcing a flush.
+	 * Allow the disks to sleep, and accept "out-of-date" statistics.
+	 * E.g. affects thin pools.  Commits will occur every second already,
+	 * if the pool state is changing.
+	 */
+	if (!dm_task_no_flush(thread->wait_task))
+		goto_out;
+
 	/* runs ioctl and may register lvm2 pluging */
 	thread->processing = 1;
 	thread->status = DM_THREAD_REGISTERING;

daemons/dmeventd/dmeventd.c +9 -0

index c093d91..5948f15 100644
@@ -412,6 +412,15 @@ static struct thread_status *_alloc_thread_status(const struct message_data *dat
 	if (!(thread->device.name = dm_strdup(data->device_uuid)))
 		goto_out;
 
+	/*
+	 * Avoid repeatedly forcing a flush.
+	 * Allow the disks to sleep, and accept "out-of-date" statistics.
+	 * E.g. affects thin pools.  Commits will occur every second already,
+	 * if the pool state is changing.
+	 */
+	if (!dm_task_no_flush(thread->wait_task))
+		goto_out;
+
 	/* runs ioctl and may register lvm2 pluging */
 	thread->processing = 1;
 	thread->status = DM_THREAD_REGISTERING;

Zdenek Kabelac

2016-01-08 08:17 UTC #1 lore raw

Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):

Quoted 5 lines

Hi
I tried using Docker on my Fedora NAS box. It created a thin pool LV, which caused hard drive activity every ~10 seconds.

Quoted 9 lines

dmeventd queries the thin pool every 10 seconds, and it causes a transaction commit in order to make sure the statistics are up to date. But transactions are already supposed to be committed after 1 second. (See Documentation/device-mapper/thin-provisioning.txt, "Updating on-disk metadata").
It seems like a simple case of "don't do that". The kernel already lets us avoid the commit. How about it (patch below)? If it seems reasonable, I can whip up a commit message for it.

I believe it's already solved upstream in version 2.02.133 of lvm2 package with this commit:

81e9ab3156badecc6a64447708c4ae4886e3c244
Date: Thu Oct 22 12:36:25 2015 +0200

Which version of lvm2 are you using ?

Regards

Zdenek

Alan Jenkins

author

2016-01-09 10:07 UTC #2 lore raw

In reply to Alan Jenkins

Quoted 27 lines · Zdenek Kabelac

Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
Quoted 14 lines
Hi
I tried using Docker on my Fedora NAS box. It created a thin pool LV, which
caused hard drive activity every ~10 seconds.
dmeventd queries the thin pool every 10 seconds, and it causes a transaction
commit in order to make sure the statistics are up to date. But transactions
are already supposed to be committed after 1 second. (See Documentation/device-mapper/thin-provisioning.txt, "Updating on-disk metadata").
Quoted 6 lines
It seems like a simple case of "don't do that". The kernel already lets us
avoid the commit. How about it (patch below)? If it seems reasonable, I can
whip up a commit message for it.
Hi
I believe it's already solved upstream in version 2.02.133 of lvm2 package with this commit:

Quoted 8 lines

81e9ab3156badecc6a64447708c4ae4886e3c244
Date: Thu Oct 22 12:36:25 2015 +0200
Which version of lvm2 are you using ?
Regards
Zdenek

Thanks! That explains a question I had.

My patch was based on lvm2 master. The upstream commit applies to the _status_ task, but I applied dm_task_no_flush() to the _wait_ task: DM_DEVICE_WAITEVENT / DM_DEV_WAIT_CMD.

I need to test this again, and I shall. (I started testing with version lvm2-2.02.132-2.fc23.x86_64).

But from the code, it looks like we need *both* patches to fix the problem. See:

1. dm-ioctl.c: dev_wait() seems to include the exact same code as dev_table_status, specifically the call to __dev_status():

1192 /*
1193 * The userland program is going to want to know what 1194 * changed to trigger the event, so we may as well tell 1195 * him and save an ioctl.
1196 */
1197 __dev_status(md, param);
1198
1199 table = dm_get_live_or_inactive_table(md, param, &srcu_idx); 1200 if (table)
1201 retrieve_status(table, param, param_size);
1202 dm_put_live_table(md, srcu_idx);

2. Consequently, dmsetup wait accepts --noflush just like dmsetup status does. (dmsetup table could as well, I don't know why it doesn't, at least not in the documentation).

Thanks again
Alan

Alan Jenkins

author

2016-01-09 11:51 UTC #3 lore raw

Quoted 39 lines · Alan Jenkins

Quoted 27 lines · Zdenek Kabelac
Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
Quoted 14 lines
Hi
I tried using Docker on my Fedora NAS box. It created a thin pool LV, which
caused hard drive activity every ~10 seconds.
dmeventd queries the thin pool every 10 seconds, and it causes a transaction
commit in order to make sure the statistics are up to date. But transactions
are already supposed to be committed after 1 second. (See Documentation/device-mapper/thin-provisioning.txt, "Updating on-disk metadata").
Quoted 6 lines
It seems like a simple case of "don't do that". The kernel already lets us
avoid the commit. How about it (patch below)? If it seems reasonable, I can
whip up a commit message for it.
Hi
I believe it's already solved upstream in version 2.02.133 of lvm2 package with this commit:
Quoted 8 lines
81e9ab3156badecc6a64447708c4ae4886e3c244
Date: Thu Oct 22 12:36:25 2015 +0200
Which version of lvm2 are you using ?
Regards
Zdenek
Thanks! That explains a question I had.

Quoted 8 lines

My patch was based on lvm2 master. The upstream commit applies to the _status_ task, but I applied dm_task_no_flush() to the _wait_ task: DM_DEVICE_WAITEVENT / DM_DEV_WAIT_CMD.
I need to test this again, and I shall. (I started testing with version lvm2-2.02.132-2.fc23.x86_64).
But from the code, it looks like we need *both* patches to fix the problem. See:
1. dm-ioctl.c: dev_wait() seems to include the exact same code as dev_table_status, specifically the call to __dev_status():

Nevermind. For me, the upstream commit fixes the problem on its own.

The wait task does get run every 10 seconds. But the 10 second timeout interrupts it before __dev_status() is called. So setting noflush on the status task, had already fixed the problem.

Thanks
Alan

P.S. if anyone wants to take my comment describing the problem, they're still welcome :).

/*
* Avoid repeatedly forcing a flush.
*
* Allow the disks to sleep, and accept "out-of-date" statistics.
* E.g. affects thin pools. Commits will occur every second already,
* if the pool state is changing.
*/

Zdenek Kabelac

2016-01-11 09:21 UTC #4 lore raw

Dne 9.1.2016 v 12:51 Alan Jenkins napsal(a):

Quoted 51 lines · Alan Jenkins

Quoted 50 lines · Zdenek Kabelac
Quoted 27 lines
Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
Quoted 14 lines
Hi
I tried using Docker on my Fedora NAS box. It created a thin pool LV, which
caused hard drive activity every ~10 seconds.
dmeventd queries the thin pool every 10 seconds, and it causes a transaction
commit in order to make sure the statistics are up to date. But transactions
are already supposed to be committed after 1 second. (See Documentation/device-mapper/thin-provisioning.txt, "Updating on-disk metadata").
Quoted 6 lines
It seems like a simple case of "don't do that". The kernel already lets us
avoid the commit. How about it (patch below)? If it seems reasonable, I can
whip up a commit message for it.
Hi
I believe it's already solved upstream in version 2.02.133 of lvm2 package with this commit:
Quoted 8 lines
81e9ab3156badecc6a64447708c4ae4886e3c244
Date: Thu Oct 22 12:36:25 2015 +0200
Which version of lvm2 are you using ?
Regards
Zdenek
Thanks! That explains a question I had.
My patch was based on lvm2 master. The upstream commit applies to the _status_ task, but I applied dm_task_no_flush() to the _wait_ task:
DM_DEVICE_WAITEVENT / DM_DEV_WAIT_CMD.
I need to test this again, and I shall. (I started testing with version lvm2-2.02.132-2.fc23.x86_64).
But from the code, it looks like we need *both* patches to fix the problem. See:

Quoted 9 lines

Quoted 2 lines
1. dm-ioctl.c: dev_wait() seems to include the exact same code as dev_table_status, specifically the call to __dev_status():
Nevermind. For me, the upstream commit fixes the problem on its own.
The wait task does get run every 10 seconds. But the 10 second timeout interrupts it before __dev_status() is called. So setting noflush on the status task, had already fixed the problem.

Status with flush on thin-pool is supposed to give you 'accurate' value, while status without flush is giving you some 'possibly out-of-date' value since not all IO are on disk (possibly the one which may cause provisioning).

However for dmeventd we should be quite happy with no-flush variant.

AFAIK I guess even the kernel has been slightly improved here, so if there is nothing to write, it should also skip disk sync in this case (even if 'flush' would have remained to be set).

And while 'disk' sleeping was one of the reasons to add this flag, the 'core' reason why 'no-flush' basically has to be ATM used with every lvm2/dmeventd status call is the blocking nature of this flushing when thin-pool gets overfilled (suspend/status needs to be able to return with some appropriate return code for this). We need to think about some better ways here - but so far this is 'reasonably' good workaround.

Regard

Zdenek