cifs: fix broken GFP_NOFS usage
PATCH 1/1 cifs: fix broken GFP_NOFS usage
Some memory allocations in cifs use GFP_KERNEL | GFP_NOFS as gfs flags but GFP_KERNEL | GFP_NOFS equals to GFP_KERNEL. So these GFP_NOFS have no effect.
This patch fixes these flags and also removes unnecessary casts to mempool_alloc.
--- fs/cifs/misc.c | 6 ++---- fs/cifs/transport.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-)
fs/cifs/misc.c +2 -4
@@ -150,8 +150,7 @@ cifs_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_req_poolp,GFP_NOFS );/* clear the first few header bytes */ /* for most paths, more is cleared in header_assemble */
@@ -188,8 +187,7 @@ cifs_small_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_sm_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_sm_req_poolp,GFP_NOFS );if (ret_buf) {/* No need to clear memory here, cleared in header assemble */ /* memset(ret_buf, 0, sizeof(struct smb_hdr) + 27);*/
fs/cifs/transport.c +1 -2
@@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smbreturn NULL ; } - temp = (struct mid_q_entry *)mempool_alloc (cifs_mid_poolp, -GFP_KERNEL |GFP_NOFS ); + temp =mempool_alloc (cifs_mid_poolp,GFP_NOFS );if (temp ==NULL )return temp;else {
Am Samstag, 24. Mai 2008 schrieb Akinobu Mita:
Quoted 7 lines
Some memory allocations in cifs use
GFP_KERNEL|GFP_NOFSas gfs flags butGFP_KERNEL|GFP_NOFSequals toGFP_KERNEL. So theseGFP_NOFShave no effect.This patch fixes these flags and also removes unnecessary casts to
mempool_alloc.
Quoted 7 lines
Signed-off-by Akinobu Mita <[email protected]>Cc Steve French <[email protected]>--- fs/cifs/misc.c | 6 ++---- fs/cifs/transport.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-)
your description above is right, but your patch is wrong (possibly a typo).
From gfp.h:
#define GFP_NOFS (__GFP_WAIT | __GFP_IO)
#define GFP_KERNEL (__GFP_WAIT | __GFP_IO | __GFP_FS)
So the current used "GFP_KERNEL | GFP_NOFS" could simply read "GFP_KERNEL".
But in your patch you are using "GFP_NOFS" instead. Cheers, Günter
Quoted 39 lines
Index:
2.6-git/fs/cifs/misc.c
===================================================================fs/cifs/misc.c +2
-4@@ -150,8 +150,7 @@ cifs_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_req_poolp,GFP_NOFS );/* clear the first few header bytes */ /* for most paths, more is cleared in header_assemble */ @@ -188,8 +187,7 @@ cifs_small_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_sm_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_sm_req_poolp,GFP_NOFS );if (ret_buf) {/* No need to clear memory here, cleared in header assemble */ /* memset(ret_buf, 0, sizeof(struct smb_hdr) + 27);*/ Index:
2.6-git/fs/cifs/transport.c
===================================================================fs/cifs/transport.c +1
-2@@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smbreturn NULL ; } - temp = (struct mid_q_entry *)mempool_alloc (cifs_mid_poolp, -GFP_KERNEL |GFP_NOFS ); + temp =mempool_alloc (cifs_mid_poolp,GFP_NOFS );if (temp ==NULL )return temp;else {
Quoted 5 lines
_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Replacing
GFP_KERNEL | GFP_NOFS
with
GFP_KERNEL
would keep the behavior the same, but that is not what was desired and presumably could cause deadlock.
There are three places that cifs was using the "conflicting" flags that Akinobu Mita noticed:
1) in allocate_mid: In this case we are holding a semaphore for the cifs socket so no page out requests of dirty pages could occur (writepage(s) calling SMB Write) to this server if we allowed the kmalloc call to go into the FS to get more free memory
2) in cifs_small_buf_get and cifs_buf_get we may be able to have GFP_FS flag on (GFP_KERNEL) - but there are still cases in which we could recurse into cifs in a path in which we already are in write
trying to free memory. This is less of a risk with cifs_buf_get
(since the default write path unless we have smb packet signing on is to use small buffers).
What was intended is GFP_NOFS in these three cases so that we would prevent kmalloc from recursing back into cifs
Quoted 75 lines · Günter Kukkukk
Am Samstag, 24. Mai 2008 schrieb Akinobu Mita:
Quoted 7 lines
Some memory allocations in cifs use
GFP_KERNEL|GFP_NOFSas gfs flags butGFP_KERNEL|GFP_NOFSequals toGFP_KERNEL. So theseGFP_NOFShave no effect.This patch fixes these flags and also removes unnecessary casts to
mempool_alloc.Quoted 7 lines
Signed-off-by Akinobu Mita <[email protected]>Cc Steve French <[email protected]>--- fs/cifs/misc.c | 6 ++---- fs/cifs/transport.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-)your description above is right, but your patch is wrong (possibly a typo).
From
gfp.h:#define GFP_NOFS (__GFP_WAIT | __GFP_IO) #define GFP_KERNEL (__GFP_WAIT | __GFP_IO | __GFP_FS)So the current used "
GFP_KERNEL|GFP_NOFS" could simply read "GFP_KERNEL".
But in your patch you are using "GFP_NOFS" instead. Cheers, GünterQuoted 44 lines
Index:
2.6-git/fs/cifs/misc.c
===================================================================fs/cifs/misc.c +2
-4@@ -150,8 +150,7 @@ cifs_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_req_poolp,GFP_NOFS );/* clear the first few header bytes */ /* for most paths, more is cleared in header_assemble */ @@ -188,8 +187,7 @@ cifs_small_buf_get(void)but it may be more efficient to always alloc same size albeit slightly larger than necessary and maxbuffersize defaults to this and can not be bigger */ - ret_buf = (struct smb_hdr *)mempool_alloc (cifs_sm_req_poolp, -GFP_KERNEL |GFP_NOFS ); + ret_buf =mempool_alloc (cifs_sm_req_poolp,GFP_NOFS );if (ret_buf) {/* No need to clear memory here, cleared in header assemble */ /* memset(ret_buf, 0, sizeof(struct smb_hdr) + 27);*/ Index:
2.6-git/fs/cifs/transport.c
===================================================================fs/cifs/transport.c +1
-2@@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smbreturn NULL ; } - temp = (struct mid_q_entry *)mempool_alloc (cifs_mid_poolp, -GFP_KERNEL |GFP_NOFS ); + temp =mempool_alloc (cifs_mid_poolp,GFP_NOFS );if (temp ==NULL )return temp;else {_______________________________________________
linux-cifs-client mailing list
[email protected]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Signature · 7 lines
Thanks, Steve -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html