[RFC] disk doesn't spin down with thin pool + dmeventd

[RFC] disk doesn't spin down with thin pool + dmeventd

[Alan Jenkins]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

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").

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

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index c093d91..5948f15 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -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;



[-- Attachment #2: dmevent-noflush.patch --]
[-- Type: text/x-patch, Size: 749 bytes --]

diff --git a/daemons/dmeventd/dmeventd.c b/daemons/dmeventd/dmeventd.c
index c093d91..5948f15 100644
--- a/daemons/dmeventd/dmeventd.c
+++ b/daemons/dmeventd/dmeventd.c
@@ -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;

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [RFC] disk doesn't spin down with thin pool + dmeventd

[Zdenek Kabelac]

Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
> 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").
>
> 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:

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

Which version of lvm2 are you using ?

Regards

Zdenek

Re: [RFC] disk doesn't spin down with thin pool + dmeventd

[Alan Jenkins]

On 08/01/16 08:17, Zdenek Kabelac wrote:
 > Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
 >> 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").
 >>
 >> 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:
 >
 > 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

Re: [RFC] disk doesn't spin down with thin pool + dmeventd

[Alan Jenkins]

On 09/01/16 10:07, Alan Jenkins wrote:
> On 08/01/16 08:17, Zdenek Kabelac wrote:
> > Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
> >> 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").
> >>
> >> 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:
> >
> > 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():

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.
	 */

Re: [RFC] disk doesn't spin down with thin pool + dmeventd

[Zdenek Kabelac]

Dne 9.1.2016 v 12:51 Alan Jenkins napsal(a):
> On 09/01/16 10:07, Alan Jenkins wrote:
>> On 08/01/16 08:17, Zdenek Kabelac wrote:
>> > Dne 7.1.2016 v 20:31 Alan Jenkins napsal(a):
>> >> 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").
>> >>
>> >> 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:
>> >
>> > 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():
>
> 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