Re: [dm-devel] dm bufio: fix deadlock issue with loop device
On 7/8/19 7:14 AM, Mike Snitzer wrote: On Fri, Jul 05 2019 at 4:24pm -0400, Junxiao Bi wrote: Hi Mike, Do i make sense on this? No, you haven't made your chase for this change. Sorry. Please refine the patch header to _not_ get into context you have from a vendor kernel. I know you say this is hard to reproduce, etc. Thanks, I will refine it in v2. But you don't even get into ther usecase where the issue was seen. Was this DM thinp? DM cache? Something else? it's thin-provision target. Customer is using docker. Please be as concise and precise as possible. Saying that shrinker is the same context as loop doesn't imply a whole lot to me (relative to why this is prone to deadlock). To restate my concern: if __GFP_FS isn't set then why does your patch help at all? If __GFP_FS is set, then that changes things.. If __GFP_FS isn't set, the behavior is the same with w/o this patch. If it is set and the mutex was already hold by others, shrinker stop, deadlock avoid. Thanks, Junxiao. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start
> -//typedef int daddr_t; /* or long - check */
> -
> struct solaris_x86_slice {
> unsigned short s_tag; /* ID tag of partition */
> unsigned short s_flag; /* permission flags */
> - longs_start;/* start sector no of partition */
> + __kernel_daddr_t s_start; /* start sector no of partition */
> longs_size; /* # of blocks in partition */
> };
What this really should use is fixed size types.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm bufio: fix deadlock issue with loop device
On Mon, Jul 08 2019 at 7:54pm -0400, Junxiao Bi wrote: > On 7/8/19 7:14 AM, Mike Snitzer wrote: > > >On Fri, Jul 05 2019 at 4:24pm -0400, > >Junxiao Bi wrote: > > > >>Hi Mike, > >> > >>Do i make sense on this? > >No, you haven't made your chase for this change. Sorry. > > > >Please refine the patch header to _not_ get into context you have from > >a vendor kernel. I know you say this is hard to reproduce, etc. > Thanks, I will refine it in v2. > >But > >you don't even get into ther usecase where the issue was seen. Was this > >DM thinp? DM cache? Something else? > it's thin-provision target. Customer is using docker. OK, with loop files? (really hackish and poor performing but loopback enabled the ability to not reinstall, or plan ahead, caused a lot of people to use it... that is until overlayfs arrived) > >Please be as concise and precise as possible. Saying that shrinker is > >the same context as loop doesn't imply a whole lot to me (relative to > >why this is prone to deadlock). > > > >To restate my concern: if __GFP_FS isn't set then why does your patch > >help at all? If __GFP_FS is set, then that changes things.. > > If __GFP_FS isn't set, the behavior is the same with w/o this patch. Yes. > If it is set and the mutex was already hold by others, shrinker > stop, deadlock avoid. Fine, please explain how that happens in the context of existing upstream code. Please make the case for fixing upstream. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start
It was meant to be used daddr_t (which is mostly int, only sparc and
mips have it defined as int), but instead used long.
But musl libc does not define daddr_t as it's deprecated, therefore
use __kernel_daddr_t from .
Signed-off-by: Petr Vorel
---
kpartx/solaris.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/kpartx/solaris.c b/kpartx/solaris.c
index 8c1a971b..e7826c62 100644
--- a/kpartx/solaris.c
+++ b/kpartx/solaris.c
@@ -1,17 +1,15 @@
#include "kpartx.h"
#include
-#include
+#include
#include /* time_t */
#define SOLARIS_X86_NUMSLICE 8
#define SOLARIS_X86_VTOC_SANE (0x600DDEEEUL)
-//typedef int daddr_t; /* or long - check */
-
struct solaris_x86_slice {
unsigned short s_tag; /* ID tag of partition */
unsigned short s_flag; /* permission flags */
- longs_start;/* start sector no of partition */
+ __kernel_daddr_t s_start; /* start sector no of partition */
longs_size; /* # of blocks in partition */
};
--
2.20.1
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] kpartx: recognize DASD on loop devices again
Thanks for the review.
Martin, can you post a rebased version of this patch ?
Thanks,
Christophe
On Mon, Jul 8, 2019 at 4:27 PM Benjamin Marzinski
wrote:
> On Fri, Jun 28, 2019 at 07:29:41PM +, Martin Wilck wrote:
> > From: Martin Wilck
> >
> > Since 4d57b868, DASD partition tables are only recognized on
> > DASD hardware. This turns out to break certain software that works
> > e.g. with DASD partition tables on loop devices. The problem that
> > 4d57b868 attempted to fix was that "unlabeled disk" format has
> > no signature at all and is detected on any volume, including
> > empty ones.
> >
> > With this patch, DASD partition table formats other than "unlabeled
> > disk" are detected on non-DASD devices again.
> >
> > Fixes: 4d57b868 "kpartx: only recognize dasd part table on DASD"
>
> Reviewed-by: Benjamin Marzinski
>
> > ---
> > kpartx/dasd.c | 7 ---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> > index 61b609a5..3fcef6ad 100644
> > --- a/kpartx/dasd.c
> > +++ b/kpartx/dasd.c
> > @@ -133,9 +133,6 @@ read_dasd_pt(int fd, struct slice all, struct slice
> *sp, int ns)
> > /* Couldn't open the device */
> > return -1;
> > }
> > - } else if ((unsigned int)major(sbuf.st_rdev) != 94) {
> > - /* Not a DASD */
> > - return -1;
> > } else {
> > fd_dasd = dup(fd);
> > }
> > @@ -277,6 +274,10 @@ read_dasd_pt(int fd, struct slice all, struct slice
> *sp, int ns)
> > size = disksize;
> > if (fmt_size < size)
> > size = fmt_size;
> > + } else if ((unsigned int)major(sbuf.st_rdev) != 94) {
> > + /* Not a DASD */
> > + retval = -1;
> > + goto out;
> > } else
> > size = disksize;
> >
> > --
> > 2.21.0
> >
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] kpartx: recognize DASD on loop devices again
On Fri, Jun 28, 2019 at 07:29:41PM +, Martin Wilck wrote:
> From: Martin Wilck
>
> Since 4d57b868, DASD partition tables are only recognized on
> DASD hardware. This turns out to break certain software that works
> e.g. with DASD partition tables on loop devices. The problem that
> 4d57b868 attempted to fix was that "unlabeled disk" format has
> no signature at all and is detected on any volume, including
> empty ones.
>
> With this patch, DASD partition table formats other than "unlabeled
> disk" are detected on non-DASD devices again.
>
> Fixes: 4d57b868 "kpartx: only recognize dasd part table on DASD"
Reviewed-by: Benjamin Marzinski
> ---
> kpartx/dasd.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/kpartx/dasd.c b/kpartx/dasd.c
> index 61b609a5..3fcef6ad 100644
> --- a/kpartx/dasd.c
> +++ b/kpartx/dasd.c
> @@ -133,9 +133,6 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp,
> int ns)
> /* Couldn't open the device */
> return -1;
> }
> - } else if ((unsigned int)major(sbuf.st_rdev) != 94) {
> - /* Not a DASD */
> - return -1;
> } else {
> fd_dasd = dup(fd);
> }
> @@ -277,6 +274,10 @@ read_dasd_pt(int fd, struct slice all, struct slice *sp,
> int ns)
> size = disksize;
> if (fmt_size < size)
> size = fmt_size;
> + } else if ((unsigned int)major(sbuf.st_rdev) != 94) {
> + /* Not a DASD */
> + retval = -1;
> + goto out;
> } else
> size = disksize;
>
> --
> 2.21.0
>
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] dm bufio: fix deadlock issue with loop device
On Fri, Jul 05 2019 at 4:24pm -0400, Junxiao Bi wrote: > Hi Mike, > > Do i make sense on this? No, you haven't made your chase for this change. Sorry. Please refine the patch header to _not_ get into context you have from a vendor kernel. I know you say this is hard to reproduce, etc. But you don't even get into ther usecase where the issue was seen. Was this DM thinp? DM cache? Something else? Please be as concise and precise as possible. Saying that shrinker is the same context as loop doesn't imply a whole lot to me (relative to why this is prone to deadlock). To restate my concern: if __GFP_FS isn't set then why does your patch help at all? If __GFP_FS is set, then that changes things.. Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
