[PATCH] cifs: fix broken GFP_NOFS usage

[PATCH] cifs: fix broken GFP_NOFS usage

[Akinobu Mita]

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.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Steve French <sfrench@samba.org>
Cc: linux-cifs-client@lists.samba.org
---
 fs/cifs/misc.c      |    6 ++----
 fs/cifs/transport.c |    3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

Index: 2.6-git/fs/cifs/misc.c
===================================================================
--- 2.6-git.orig/fs/cifs/misc.c
+++ 2.6-git/fs/cifs/misc.c
@@ -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
===================================================================
--- 2.6-git.orig/fs/cifs/transport.c
+++ 2.6-git/fs/cifs/transport.c
@@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smb
 		return 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 {

Re: [linux-cifs-client] [PATCH] cifs: fix broken GFP_NOFS usage

[Günter Kukkukk]

Am Samstag, 24. Mai 2008 schrieb Akinobu Mita:
> 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.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Steve French <sfrench@samba.org>
> Cc: linux-cifs-client@lists.samba.org
> ---
>  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

> 
> Index: 2.6-git/fs/cifs/misc.c
> ===================================================================
> --- 2.6-git.orig/fs/cifs/misc.c
> +++ 2.6-git/fs/cifs/misc.c
> @@ -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
> ===================================================================
> --- 2.6-git.orig/fs/cifs/transport.c
> +++ 2.6-git/fs/cifs/transport.c
> @@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smb
>  		return 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
> linux-cifs-client@lists.samba.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [linux-cifs-client] [PATCH] cifs: fix broken GFP_NOFS usage

[Steve French]

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

On Sat, May 24, 2008 at 12:13 PM, Günter Kukkukk <linux@kukkukk.com> wrote:
> Am Samstag, 24. Mai 2008 schrieb Akinobu Mita:
>> 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.
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Cc: Steve French <sfrench@samba.org>
>> Cc: linux-cifs-client@lists.samba.org
>> ---
>>  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
>
>>
>> Index: 2.6-git/fs/cifs/misc.c
>> ===================================================================
>> --- 2.6-git.orig/fs/cifs/misc.c
>> +++ 2.6-git/fs/cifs/misc.c
>> @@ -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
>> ===================================================================
>> --- 2.6-git.orig/fs/cifs/transport.c
>> +++ 2.6-git/fs/cifs/transport.c
>> @@ -50,8 +50,7 @@ AllocMidQEntry(const struct smb_hdr *smb
>>               return 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
>> linux-cifs-client@lists.samba.org
>> https://lists.samba.org/mailman/listinfo/linux-cifs-client
>>
>
>
>



-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html