Skip to content

Commit

Permalink
cifs: when CONFIG_HIGHMEM is set, serialize the read/write kmaps
Browse files Browse the repository at this point in the history
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 [eed63db] 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]
 torvalds#8 [eed63ecc] do_sync_write at c052d202
 torvalds#9 [eed63f74] vfs_write at c052d4ee
torvalds#10 [eed63f94] sys_write at c052df4c
torvalds#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@vger.kernel.org>
Reported-by: Jian Li <jiali@redhat.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Steve French <smfrench@gmail.com>
  • Loading branch information
jtlayton authored and smfrench committed Jul 17, 2012
1 parent 3ae629d commit 3cf003c
Showing 1 changed file with 29 additions and 1 deletion.
30 changes: 29 additions & 1 deletion fs/cifs/cifssmb.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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);

Expand Down

0 comments on commit 3cf003c

Please sign in to comment.