[PATCH 0/2] cifs: fix kmap deadlock on CONFIG_HIGHMEM machines with large r/wsizes

[PATCH 0/2] cifs: fix kmap deadlock on CONFIG_HIGHMEM machines with large r/wsizes

[Jeff Layton]

The current read/write code in cifs.ko relies on being able to kmap all
of the data at once. This is a problem on machines with high memory.
kmap addresses are a limited resource and you can easily end up
exhausting it if your rsize/wsize is too large.

One such report of this problem is here:

    https://bugzilla.redhat.com/show_bug.cgi?id=832252

This patchset is intended as a temporary workaround on those machines
that should help prevent that sort of exhaustion.

The right fix for this is to teach the code how to deal with lists of
pages directly instead of relying on being able to kmap them all. I have
a patchset in progress that does just that, but it's not complete yet
and is pretty invasive. Until that's ready, I think this set is a
reasonable workaround.

Jeff Layton (2):
  cifs: on CONFIG_HIGHMEM machines, limit the rsize/wsize to the kmap
    space
  cifs: when CONFIG_HIGHMEM is set, serialize the read/write kmaps

 fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++++-
 fs/cifs/connect.c |   18 ++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] cifs: on CONFIG_HIGHMEM machines, limit the rsize/wsize to the kmap space

[Jeff Layton]

We currently rely on being able to kmap all of the pages in an async
read or write request. If you're on a machine that has CONFIG_HIGHMEM
set then that kmap space is limited, sometimes to as low as 512 slots.

With 512 slots, we can only support up to a 2M r/wsize, and that's
assuming that we can get our greedy little hands on all of them. There
are other users however, so it's possible we'll end up stuck with a
size that large.

Since we can't handle a rsize or wsize larger than that currently, cap
those options at the number of kmap slots we have. We could consider
capping it even lower, but we currently default to a max of 1M. Might as
well allow those luddites on 32 bit arches enough rope to hang
themselves.

A more robust fix would be to teach the send and receive routines how
to contend with an array of pages so we don't need to marshal up a kvec
array at all. That's a fairly significant overhaul though, so we'll need
this limit in place until that's ready.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Reported-by: Jian Li <jiali-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 278ca0e..e8c3e6b 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3445,6 +3445,18 @@ void cifs_setup_cifs_sb(struct smb_vol *pvolume_info,
 #define CIFS_DEFAULT_NON_POSIX_RSIZE (60 * 1024)
 #define CIFS_DEFAULT_NON_POSIX_WSIZE (65536)
 
+/*
+ * On hosts with high memory, we can't currently support wsize/rsize that are
+ * larger than we can kmap at once. Cap the rsize/wsize at
+ * LAST_PKMAP * PAGE_SIZE. We'll never be able to fill a read or write request
+ * larger than that anyway.
+ */
+#ifdef CONFIG_HIGHMEM
+#define CIFS_KMAP_SIZE_LIMIT	(LAST_PKMAP * PAGE_CACHE_SIZE)
+#else /* CONFIG_HIGHMEM */
+#define CIFS_KMAP_SIZE_LIMIT	(1<<24)
+#endif /* CONFIG_HIGHMEM */
+
 static unsigned int
 cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
 {
@@ -3475,6 +3487,9 @@ cifs_negotiate_wsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
 		wsize = min_t(unsigned int, wsize,
 				server->maxBuf - sizeof(WRITE_REQ) + 4);
 
+	/* limit to the amount that we can kmap at once */
+	wsize = min_t(unsigned int, wsize, CIFS_KMAP_SIZE_LIMIT);
+
 	/* hard limit of CIFS_MAX_WSIZE */
 	wsize = min_t(unsigned int, wsize, CIFS_MAX_WSIZE);
 
@@ -3516,6 +3531,9 @@ cifs_negotiate_rsize(struct cifs_tcon *tcon, struct smb_vol *pvolume_info)
 	if (!(server->capabilities & CAP_LARGE_READ_X))
 		rsize = min_t(unsigned int, CIFSMaxBufSize, rsize);
 
+	/* limit to the amount that we can kmap at once */
+	rsize = min_t(unsigned int, rsize, CIFS_KMAP_SIZE_LIMIT);
+
 	/* hard limit of CIFS_MAX_RSIZE */
 	rsize = min_t(unsigned int, rsize, CIFS_MAX_RSIZE);
 
-- 
1.7.7.6

[PATCH 2/2] cifs: when CONFIG_HIGHMEM is set, serialize the read/write kmaps

[Jeff Layton]

Jian found that when he ran fsx on a 32 bit arch with a large wsize the
process and one of the bdi writeback kthreads would sometimes deadlock
with a stack trace like this:

crash> bt
PID: 2789   TASK: f02edaa0  CPU: 3   COMMAND: "fsx"
 #0 [eed63cbc] schedule at c083c5b3
 #1 [eed63d80] kmap_high at c0500ec8
 #2 [eed63db0] cifs_async_writev at f7fabcd7 [cifs]
 #3 [eed63df0] cifs_writepages at f7fb7f5c [cifs]
 #4 [eed63e50] do_writepages at c04f3e32
 #5 [eed63e54] __filemap_fdatawrite_range at c04e152a
 #6 [eed63ea4] filemap_fdatawrite at c04e1b3e
 #7 [eed63eb4] cifs_file_aio_write at f7fa111a [cifs]
 #8 [eed63ecc] do_sync_write at c052d202
 #9 [eed63f74] vfs_write at c052d4ee
#10 [eed63f94] sys_write at c052df4c
#11 [eed63fb0] ia32_sysenter_target at c0409a98
    EAX: 00000004  EBX: 00000003  ECX: abd73b73  EDX: 012a65c6
    DS:  007b      ESI: 012a65c6  ES:  007b      EDI: 00000000
    SS:  007b      ESP: bf8db178  EBP: bf8db1f8  GS:  0033
    CS:  0073      EIP: 40000424  ERR: 00000004  EFLAGS: 00000246

Each task would kmap part of its address array before getting stuck, but
not enough to actually issue the write.

This patch fixes this by serializing the marshal_iov operations for
async reads and writes. The idea here is to ensure that cifs
aggressively tries to populate a request before attempting to fulfill
another one. As soon as all of the pages are kmapped for a request, then
we can unlock and allow another one to proceed.

There's no need to do this serialization on non-CONFIG_HIGHMEM arches
however, so optimize all of this out when CONFIG_HIGHMEM isn't set.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Reported-by: Jian Li <jiali-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 0170ee8..684a072 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -86,7 +86,31 @@ static struct {
 #endif /* CONFIG_CIFS_WEAK_PW_HASH */
 #endif /* CIFS_POSIX */
 
-/* Forward declarations */
+#ifdef CONFIG_HIGHMEM
+/*
+ * On arches that have high memory, kmap address space is limited. By
+ * serializing the kmap operations on those arches, we ensure that we don't
+ * end up with a bunch of threads in writeback with partially mapped page
+ * arrays, stuck waiting for kmap to come back. That situation prevents
+ * progress and can deadlock.
+ */
+static DEFINE_MUTEX(cifs_kmap_mutex);
+
+static inline void
+cifs_kmap_lock(void)
+{
+	mutex_lock(&cifs_kmap_mutex);
+}
+
+static inline void
+cifs_kmap_unlock(void)
+{
+	mutex_unlock(&cifs_kmap_mutex);
+}
+#else /* !CONFIG_HIGHMEM */
+#define cifs_kmap_lock() do { ; } while(0)
+#define cifs_kmap_unlock() do { ; } while(0)
+#endif /* CONFIG_HIGHMEM */
 
 /* Mark as invalid, all open files on tree connections since they
    were closed when session to server was lost */
@@ -1503,7 +1527,9 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 	}
 
 	/* marshal up the page array */
+	cifs_kmap_lock();
 	len = rdata->marshal_iov(rdata, data_len);
+	cifs_kmap_unlock();
 	data_len -= len;
 
 	/* issue the read if we have any iovecs left to fill */
@@ -2069,7 +2095,9 @@ cifs_async_writev(struct cifs_writedata *wdata)
 	 * and set the iov_len properly for each one. It may also set
 	 * wdata->bytes too.
 	 */
+	cifs_kmap_lock();
 	wdata->marshal_iov(iov, wdata);
+	cifs_kmap_unlock();
 
 	cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
 
-- 
1.7.7.6

Re: [PATCH 2/2] cifs: when CONFIG_HIGHMEM is set, serialize the read/write kmaps

[Pavel Shilovsky]

2012/7/11 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> Jian found that when he ran fsx on a 32 bit arch with a large wsize the
> process and one of the bdi writeback kthreads would sometimes deadlock
> with a stack trace like this:
>
> crash> bt
> PID: 2789   TASK: f02edaa0  CPU: 3   COMMAND: "fsx"
>  #0 [eed63cbc] schedule at c083c5b3
>  #1 [eed63d80] kmap_high at c0500ec8
>  #2 [eed63db0] cifs_async_writev at f7fabcd7 [cifs]
>  #3 [eed63df0] cifs_writepages at f7fb7f5c [cifs]
>  #4 [eed63e50] do_writepages at c04f3e32
>  #5 [eed63e54] __filemap_fdatawrite_range at c04e152a
>  #6 [eed63ea4] filemap_fdatawrite at c04e1b3e
>  #7 [eed63eb4] cifs_file_aio_write at f7fa111a [cifs]
>  #8 [eed63ecc] do_sync_write at c052d202
>  #9 [eed63f74] vfs_write at c052d4ee
> #10 [eed63f94] sys_write at c052df4c
> #11 [eed63fb0] ia32_sysenter_target at c0409a98
>     EAX: 00000004  EBX: 00000003  ECX: abd73b73  EDX: 012a65c6
>     DS:  007b      ESI: 012a65c6  ES:  007b      EDI: 00000000
>     SS:  007b      ESP: bf8db178  EBP: bf8db1f8  GS:  0033
>     CS:  0073      EIP: 40000424  ERR: 00000004  EFLAGS: 00000246
>
> Each task would kmap part of its address array before getting stuck, but
> not enough to actually issue the write.
>
> This patch fixes this by serializing the marshal_iov operations for
> async reads and writes. The idea here is to ensure that cifs
> aggressively tries to populate a request before attempting to fulfill
> another one. As soon as all of the pages are kmapped for a request, then
> we can unlock and allow another one to proceed.
>
> There's no need to do this serialization on non-CONFIG_HIGHMEM arches
> however, so optimize all of this out when CONFIG_HIGHMEM isn't set.
>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Reported-by: Jian Li <jiali-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++++-
>  1 files changed, 29 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 0170ee8..684a072 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -86,7 +86,31 @@ static struct {
>  #endif /* CONFIG_CIFS_WEAK_PW_HASH */
>  #endif /* CIFS_POSIX */
>
> -/* Forward declarations */
> +#ifdef CONFIG_HIGHMEM
> +/*
> + * On arches that have high memory, kmap address space is limited. By
> + * serializing the kmap operations on those arches, we ensure that we don't
> + * end up with a bunch of threads in writeback with partially mapped page
> + * arrays, stuck waiting for kmap to come back. That situation prevents
> + * progress and can deadlock.
> + */
> +static DEFINE_MUTEX(cifs_kmap_mutex);
> +
> +static inline void
> +cifs_kmap_lock(void)
> +{
> +       mutex_lock(&cifs_kmap_mutex);
> +}
> +
> +static inline void
> +cifs_kmap_unlock(void)
> +{
> +       mutex_unlock(&cifs_kmap_mutex);
> +}
> +#else /* !CONFIG_HIGHMEM */
> +#define cifs_kmap_lock() do { ; } while(0)

checkpatch.pl raises the "ERROR: space required before the open
parenthesis '('" error.

> +#define cifs_kmap_unlock() do { ; } while(0)

the same checkpatch.pl error.

> +#endif /* CONFIG_HIGHMEM */
>
>  /* Mark as invalid, all open files on tree connections since they
>     were closed when session to server was lost */
> @@ -1503,7 +1527,9 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
>         }
>
>         /* marshal up the page array */
> +       cifs_kmap_lock();
>         len = rdata->marshal_iov(rdata, data_len);
> +       cifs_kmap_unlock();
>         data_len -= len;
>
>         /* issue the read if we have any iovecs left to fill */
> @@ -2069,7 +2095,9 @@ cifs_async_writev(struct cifs_writedata *wdata)
>          * and set the iov_len properly for each one. It may also set
>          * wdata->bytes too.
>          */
> +       cifs_kmap_lock();
>         wdata->marshal_iov(iov, wdata);
> +       cifs_kmap_unlock();
>
>         cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
>
> --
> 1.7.7.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH 2/2] cifs: when CONFIG_HIGHMEM is set, serialize the read/write kmaps

[Jeff Layton]

On Thu, 12 Jul 2012 20:41:19 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2012/7/11 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> > Jian found that when he ran fsx on a 32 bit arch with a large wsize the
> > process and one of the bdi writeback kthreads would sometimes deadlock
> > with a stack trace like this:
> >
> > crash> bt
> > PID: 2789   TASK: f02edaa0  CPU: 3   COMMAND: "fsx"
> >  #0 [eed63cbc] schedule at c083c5b3
> >  #1 [eed63d80] kmap_high at c0500ec8
> >  #2 [eed63db0] cifs_async_writev at f7fabcd7 [cifs]
> >  #3 [eed63df0] cifs_writepages at f7fb7f5c [cifs]
> >  #4 [eed63e50] do_writepages at c04f3e32
> >  #5 [eed63e54] __filemap_fdatawrite_range at c04e152a
> >  #6 [eed63ea4] filemap_fdatawrite at c04e1b3e
> >  #7 [eed63eb4] cifs_file_aio_write at f7fa111a [cifs]
> >  #8 [eed63ecc] do_sync_write at c052d202
> >  #9 [eed63f74] vfs_write at c052d4ee
> > #10 [eed63f94] sys_write at c052df4c
> > #11 [eed63fb0] ia32_sysenter_target at c0409a98
> >     EAX: 00000004  EBX: 00000003  ECX: abd73b73  EDX: 012a65c6
> >     DS:  007b      ESI: 012a65c6  ES:  007b      EDI: 00000000
> >     SS:  007b      ESP: bf8db178  EBP: bf8db1f8  GS:  0033
> >     CS:  0073      EIP: 40000424  ERR: 00000004  EFLAGS: 00000246
> >
> > Each task would kmap part of its address array before getting stuck, but
> > not enough to actually issue the write.
> >
> > This patch fixes this by serializing the marshal_iov operations for
> > async reads and writes. The idea here is to ensure that cifs
> > aggressively tries to populate a request before attempting to fulfill
> > another one. As soon as all of the pages are kmapped for a request, then
> > we can unlock and allow another one to proceed.
> >
> > There's no need to do this serialization on non-CONFIG_HIGHMEM arches
> > however, so optimize all of this out when CONFIG_HIGHMEM isn't set.
> >
> > Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> > Reported-by: Jian Li <jiali-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  fs/cifs/cifssmb.c |   30 +++++++++++++++++++++++++++++-
> >  1 files changed, 29 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index 0170ee8..684a072 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -86,7 +86,31 @@ static struct {
> >  #endif /* CONFIG_CIFS_WEAK_PW_HASH */
> >  #endif /* CIFS_POSIX */
> >
> > -/* Forward declarations */
> > +#ifdef CONFIG_HIGHMEM
> > +/*
> > + * On arches that have high memory, kmap address space is limited. By
> > + * serializing the kmap operations on those arches, we ensure that we don't
> > + * end up with a bunch of threads in writeback with partially mapped page
> > + * arrays, stuck waiting for kmap to come back. That situation prevents
> > + * progress and can deadlock.
> > + */
> > +static DEFINE_MUTEX(cifs_kmap_mutex);
> > +
> > +static inline void
> > +cifs_kmap_lock(void)
> > +{
> > +       mutex_lock(&cifs_kmap_mutex);
> > +}
> > +
> > +static inline void
> > +cifs_kmap_unlock(void)
> > +{
> > +       mutex_unlock(&cifs_kmap_mutex);
> > +}
> > +#else /* !CONFIG_HIGHMEM */
> > +#define cifs_kmap_lock() do { ; } while(0)
> 
> checkpatch.pl raises the "ERROR: space required before the open
> parenthesis '('" error.
> 
> > +#define cifs_kmap_unlock() do { ; } while(0)
> 
> the same checkpatch.pl error.
> 

Ok...

I fixed it in my cifs-next branch, but I'm not going to bother to
re-post unless Steve asks otherwise.

> > +#endif /* CONFIG_HIGHMEM */
> >
> >  /* Mark as invalid, all open files on tree connections since they
> >     were closed when session to server was lost */
> > @@ -1503,7 +1527,9 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid)
> >         }
> >
> >         /* marshal up the page array */
> > +       cifs_kmap_lock();
> >         len = rdata->marshal_iov(rdata, data_len);
> > +       cifs_kmap_unlock();
> >         data_len -= len;
> >
> >         /* issue the read if we have any iovecs left to fill */
> > @@ -2069,7 +2095,9 @@ cifs_async_writev(struct cifs_writedata *wdata)
> >          * and set the iov_len properly for each one. It may also set
> >          * wdata->bytes too.
> >          */
> > +       cifs_kmap_lock();
> >         wdata->marshal_iov(iov, wdata);
> > +       cifs_kmap_unlock();
> >
> >         cFYI(1, "async write at %llu %u bytes", wdata->offset, wdata->bytes);
> >
> > --
> > 1.7.7.6
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 


-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>