Re: [dm-devel] dm-crypt: fix lost ioprio when queuing crypto bios from task with ioprio

2016-12-29 Thread Eric Wheeler 
On Sat, 17 Dec 2016, Mike Snitzer wrote:
> On Fri, Dec 16 2016 at  5:29pm -0500,
> Eric Wheeler  wrote:
> > On Wed, 14 Dec 2016, Eric Wheeler wrote:
> > > Since dm-crypt queues writes (and sometimes reads) to a different kernel
> > > thread (workqueue), the bios will dispatch from tasks with different
> > > io_context->ioprio settings than the submitting task, thus giving
> > > incorrect ioprio hints to the io scheduler.
> > 
> > The motivation of this patch is for ioprio-driven writebackup/bypass 
> > hinting inside bcache when bcache is under dm-crypt which Jens is picking 
> > up for 4.10:
> >   https://lkml.org/lkml/2016/12/6/607
> 
> I now see your commits:
> b71de4659fba4e42c7 bcache: introduce per-process ioprio-based 
> bypass/writeback hints
> 82e7192711c3855038 bcache: documentation for ioprio cache hinting
> 
> You'd think this is the type of thing that you'd have proposed to a
> wider audience.  

( Its not really relevant to this bugfix, but please see this thread since 
  you were curious about a wider audience discussion: 
  https://www.redhat.com/archives/dm-devel/2016-July/msg00556.html )

The note above in the previous email was intended to explain how we 
discovered the dm-crypt problem, purely as an example use case.  The 
stable commit note discusses the real issue: lost elevator hints.

This commit fixes elevator ioprio hints passing through dm-crypt and is 
not intended to address dm-cache, nor enable a bcache feature.

All impementations using ioprio hints beneath dm-crypt would 
benefit---most importantly, _CFQ_ !
  
As it is, all ioprio hints passing through dm-crypt are lost to the 
elevator; the elevator looses those useful bits because of queuing to 
another thread for crypto operations.

> > The ioprio aware schedulers like CFQ and BFQ also benefit with more 
> > knowledge about the bio's when passing through dm-crypt.  It would be 
> > great if this can be accepted for 4.10, too.
> 
> It also needs more review, testing and possible re-working.  Each DM
> target shouldn't have to worry about these details (though I do grant
> that dm-crypt.c:clone_init call to bio_set_prio makes sense).

The patch is trivial: +5 lines in dm-crypt.c (excluding `if` bracing), a 
helper function in bio.h, and a 1-line fixup in bio.c.  Thus, the stable@ 
inclusion since it would probably patch down to v3.x somewhere and help 
everyone who uses dm-crypt with ionice.

Its only 4.10-rc1, and as a bugfix it could be accepted as a stable commit 
for 4.10-rc2 or later if you are willing to help dm-crypt users who use 
ionice.  Indeed, if you so choose, you could both accept this commit and 
still re-work it in 4.11.

This is to benefit everyone, using kernels both old and new.

Cheers,

-Eric


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH] multipathd: fix SIGUSR2 handling

2016-12-29 Thread tang . junhui 
From: "tang.junhui" 

SIGUSR2 is not blocked by default, it would cause bellow conflict
in removing of the last path:
-
uevent processing thread | waiter thread
-|---
uev_remove_path()|   waiteventloop()
  lock(&vecs->lock)  |
  ev_remove_path()   | 
dm_queue_if_no_path()|> dm_task_run()   
flush_map()  |  |
  remove_map_and_stop_waiter()   |  -> 
pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
_remove_map()| 
lock(&waiter->vecs->lock)//wait for the locker   
  stop_waiter_thread()   | 
pthread_cancel(thread)   |  
pthread_kill(thread,SIGUSR2)-|--> sigusr2 (int sig)
 |  condlog()
 |  fprintf() //it has test 
cancel point
 |  cleanup_lock() //thread 
cancel, and pop, which unlock the 
 |   locker of 
uevent processing thread 
--
Since SIGUSR2 is only needed when waiter thread running in dm_task_run(),
and it has already been unblocked before dm_task_run(), so this patch block 
SIGUSR2 for other times.

Change-Id: I8c46292dc954415764ebbe054498b4f7ea53c1c6
Signed-off-by: tang.junhui 
---
 multipathd/main.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index adc3258..fe69abd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2181,6 +2181,12 @@ sigusr2 (int sig)
 static void
 signal_init(void)
 {
+   sigset_t set;
+
+   sigemptyset(&set);
+   sigaddset(&set, SIGUSR2);
+   pthread_sigmask(SIG_SETMASK, &set, NULL);
+
signal_set(SIGHUP, sighup);
signal_set(SIGUSR1, sigusr1);
signal_set(SIGUSR2, sigusr2);
-- 
2.8.1.windows.1


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel