Fwd: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

Fwd: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Steve French]

Forwarding to the current cifs development mailing list


---------- Forwarded message ----------
From:  <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
Date: Wed, Feb 6, 2013 at 12:26 AM
Subject: [PATCH] cifs: bugfix for unreclaimed writeback pages in
cifs_writev_requeue()
To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>, linux-cifs-client-w/Ol4Ecudpl8XjKLYN78aQ@public.gmane.org



    This patch applies to the kernel version of linux-3.8-rc6.

    Pages get the PG_writeback flag set before cifs sends its request
to SMB server
    in cifs_writepages(), if the SMB service goes down, cifs may try
to recommit the
    writing requests in cifs_writev_requeue(). However, it does not
clean its PG_writeback
    flag and relaimed the pages even if it fails again in
cifs_writev_requeue(), which
    may lead to the hanging of the processes accessing the cifs
directory. This patch
    just cleans the PG_writeback flags and reclaims the pages under
that circumstances.

    Steps to reproduce the bug(trying serveral times may trigger the issue):
    1.Write from cifs client continuously.(e.g dd if=/dev/zero of=<cifs file>)
    2.Stop SMB service from server.(e.g service smb stop)
    3.Wait for two minutes， and then start SMB service from
server.(e.g service smb start)
    4.The processes which are accessing cifs directory may hang up.

    Signed-off-by: Ouyang Maochun <ouyang.maochun-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
    Signed-off-by: Jiang Yong <jian.yong5-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
    Tested-by: Zhang Xianwei <zhang.xianwei8-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
    Reviewed-by: Wang Liang <wang.liang82-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
    Reviewed-by: Cai Qu <cai.qu-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
    Reviewed-by: Jiang Biao <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>

diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
--- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
+++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
15:44:09.869254397 +0800
@@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
         } while (rc == -EAGAIN);

         for (i = 0; i < wdata->nr_pages; i++) {
-                if (rc != 0)
+                if (rc != 0){
                         SetPageError(wdata->pages[i]);
+                         end_page_writeback(wdata->pages[i]);
+                         page_cache_release(wdata->pages[i]);
+                 }
                 unlock_page(wdata->pages[i]);
         }


--------------------------------------------------------
ZTE Information Security Notice: The information contained in this
mail (and any attachment transmitted herewith) is privileged and
confidential and is intended for the exclusive use of the
addressee(s).  If you are not an intended recipient, any disclosure,
reproduction, distribution or other dissemination or use of the
information contained is strictly prohibited.  If you have received
this mail in error, please delete it and notify us immediately.




-- 
Thanks,

Steve

Re: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Pavel Shilovsky]

2013/2/6 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Forwarding to the current cifs development mailing list
>
>
> ---------- Forwarded message ----------
> From:  <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
> Date: Wed, Feb 6, 2013 at 12:26 AM
> Subject: [PATCH] cifs: bugfix for unreclaimed writeback pages in
> cifs_writev_requeue()
> To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>, linux-cifs-client-w/Ol4Ecudplf7j03Gpi2xA@public.gmane.orgrg
>
>
>
>     This patch applies to the kernel version of linux-3.8-rc6.
>
>     Pages get the PG_writeback flag set before cifs sends its request
> to SMB server
>     in cifs_writepages(), if the SMB service goes down, cifs may try
> to recommit the
>     writing requests in cifs_writev_requeue(). However, it does not
> clean its PG_writeback
>     flag and relaimed the pages even if it fails again in
> cifs_writev_requeue(), which
>     may lead to the hanging of the processes accessing the cifs
> directory. This patch
>     just cleans the PG_writeback flags and reclaims the pages under
> that circumstances.
>
>     Steps to reproduce the bug(trying serveral times may trigger the issue):
>     1.Write from cifs client continuously.(e.g dd if=/dev/zero of=<cifs file>)
>     2.Stop SMB service from server.(e.g service smb stop)
>     3.Wait for two minutes， and then start SMB service from
> server.(e.g service smb start)
>     4.The processes which are accessing cifs directory may hang up.
>
>     Signed-off-by: Ouyang Maochun <ouyang.maochun-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Signed-off-by: Jiang Yong <jian.yong5-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Tested-by: Zhang Xianwei <zhang.xianwei8-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Wang Liang <wang.liang82-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Cai Qu <cai.qu-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Jiang Biao <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>
> diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
> --- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
> +++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
> 15:44:09.869254397 +0800
> @@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
>          } while (rc == -EAGAIN);
>
>          for (i = 0; i < wdata->nr_pages; i++) {
> -                if (rc != 0)
> +                if (rc != 0){
>                          SetPageError(wdata->pages[i]);
> +                         end_page_writeback(wdata->pages[i]);
> +                         page_cache_release(wdata->pages[i]);
> +                 }

I think we should do end_page_writeback(wdata->pages[i]) and
page_cache_release(wdata->pages[i]) regardless whether rc is 0 or not
(we are doing the same things in cifs_writev_complete now). Thoughts?

>                  unlock_page(wdata->pages[i]);
>          }
>
>
> --------------------------------------------------------
> ZTE Information Security Notice: The information contained in this
> mail (and any attachment transmitted herewith) is privileged and
> confidential and is intended for the exclusive use of the
> addressee(s).  If you are not an intended recipient, any disclosure,
> reproduction, distribution or other dissemination or use of the
> information contained is strictly prohibited.  If you have received
> this mail in error, please delete it and notify us immediately.
>
>
>
>
> --
> Thanks,
>
> Steve
> --
> 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] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Pavel Shilovsky]

2013/2/7  <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>:
>> I think we should do end_page_writeback(wdata->pages[i]) and
>> page_cache_release(wdata->pages[i]) regardless whether rc is 0 or not
>> (we are doing the same things in cifs_writev_complete now). Thoughts?
> Actually,if rc=0 in cifs_writev_requeue, end_page_writeback(wdata->pages[i])
> and
> page_cache_release(wdata->pages[i]) have been done in cifs_writev_complete,
> and we
> should not worry about that.What we need fix here is the situation when
> rc!=0.

Ok. We go into cifs_writev_complete with wdata->sync_mode ==
WB_SYNC_ALL and wdata->result == -EAGAIN. In this case we call
cifs_write_requeue:

} else if (wdata->sync_mode == WB_SYNC_ALL && wdata->result == -EAGAIN)
        return cifs_writev_requeue(wdata);"

end_page_writeback and page_cache_release are called after the
statement above - so, if we hit in cifs_writev_requeue, they are not
called anymore. That's why we should call them from
cifs_writev_requeue unconditionally.

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Jeff Layton]

On Wed, 6 Feb 2013 12:00:08 -0600
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Forwarding to the current cifs development mailing list
> 
> 
> ---------- Forwarded message ----------
> From:  <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
> Date: Wed, Feb 6, 2013 at 12:26 AM
> Subject: [PATCH] cifs: bugfix for unreclaimed writeback pages in
> cifs_writev_requeue()
> To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>, linux-cifs-client-w/Ol4Ecudplf7j03Gpi2xA@public.gmane.orgrg
> 
> 
> 
>     This patch applies to the kernel version of linux-3.8-rc6.
> 
>     Pages get the PG_writeback flag set before cifs sends its request
> to SMB server
>     in cifs_writepages(), if the SMB service goes down, cifs may try
> to recommit the
>     writing requests in cifs_writev_requeue(). However, it does not
> clean its PG_writeback
>     flag and relaimed the pages even if it fails again in
> cifs_writev_requeue(), which
>     may lead to the hanging of the processes accessing the cifs
> directory. This patch
>     just cleans the PG_writeback flags and reclaims the pages under
> that circumstances.
> 
>     Steps to reproduce the bug(trying serveral times may trigger the issue):
>     1.Write from cifs client continuously.(e.g dd if=/dev/zero of=<cifs file>)
>     2.Stop SMB service from server.(e.g service smb stop)
>     3.Wait for two minutes， and then start SMB service from
> server.(e.g service smb start)
>     4.The processes which are accessing cifs directory may hang up.
> 
>     Signed-off-by: Ouyang Maochun <ouyang.maochun-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Signed-off-by: Jiang Yong <jian.yong5-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Tested-by: Zhang Xianwei <zhang.xianwei8-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Wang Liang <wang.liang82-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Cai Qu <cai.qu-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>     Reviewed-by: Jiang Biao <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
> 
> diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
> --- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
> +++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
> 15:44:09.869254397 +0800
> @@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
>          } while (rc == -EAGAIN);
> 
>          for (i = 0; i < wdata->nr_pages; i++) {
> -                if (rc != 0)
> +                if (rc != 0){
>                          SetPageError(wdata->pages[i]);
> +                         end_page_writeback(wdata->pages[i]);
> +                         page_cache_release(wdata->pages[i]);
> +                 }
>                  unlock_page(wdata->pages[i]);
>          }
> 
> 

Patch looks correct to me. Nice catch.

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

Re: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Steve French]

Patch merged but had to hand merge it since the original was not sent
as plain text email.

On Thu, Feb 7, 2013 at 5:39 AM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Wed, 6 Feb 2013 12:00:08 -0600
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> Forwarding to the current cifs development mailing list
>>
>>
>> ---------- Forwarded message ----------
>> From:  <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>> Date: Wed, Feb 6, 2013 at 12:26 AM
>> Subject: [PATCH] cifs: bugfix for unreclaimed writeback pages in
>> cifs_writev_requeue()
>> To: Steve French <sfrench-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>, linux-cifs-client-w/Ol4Ecudpm1eZ56f2WSdA@public.gmane.orgorg
>>
>>
>>
>>     This patch applies to the kernel version of linux-3.8-rc6.
>>
>>     Pages get the PG_writeback flag set before cifs sends its request
>> to SMB server
>>     in cifs_writepages(), if the SMB service goes down, cifs may try
>> to recommit the
>>     writing requests in cifs_writev_requeue(). However, it does not
>> clean its PG_writeback
>>     flag and relaimed the pages even if it fails again in
>> cifs_writev_requeue(), which
>>     may lead to the hanging of the processes accessing the cifs
>> directory. This patch
>>     just cleans the PG_writeback flags and reclaims the pages under
>> that circumstances.
>>
>>     Steps to reproduce the bug(trying serveral times may trigger the issue):
>>     1.Write from cifs client continuously.(e.g dd if=/dev/zero of=<cifs file>)
>>     2.Stop SMB service from server.(e.g service smb stop)
>>     3.Wait for two minutes， and then start SMB service from
>> server.(e.g service smb start)
>>     4.The processes which are accessing cifs directory may hang up.
>>
>>     Signed-off-by: Ouyang Maochun <ouyang.maochun-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>     Signed-off-by: Jiang Yong <jian.yong5-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>     Tested-by: Zhang Xianwei <zhang.xianwei8-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>     Reviewed-by: Wang Liang <wang.liang82-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>     Reviewed-by: Cai Qu <cai.qu-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>     Reviewed-by: Jiang Biao <jiang.biao2-Th6q7B73Y6EnDS1+zs4M5A@public.gmane.org>
>>
>> diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
>> --- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
>> +++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
>> 15:44:09.869254397 +0800
>> @@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
>>          } while (rc == -EAGAIN);
>>
>>          for (i = 0; i < wdata->nr_pages; i++) {
>> -                if (rc != 0)
>> +                if (rc != 0){
>>                          SetPageError(wdata->pages[i]);
>> +                         end_page_writeback(wdata->pages[i]);
>> +                         page_cache_release(wdata->pages[i]);
>> +                 }
>>                  unlock_page(wdata->pages[i]);
>>          }
>>
>>
>
> Patch looks correct to me. Nice catch.
>
> Reviewed-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>



-- 
Thanks,

Steve

Re: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Jeff Layton]

On Fri, 1 Mar 2013 22:08:04 +0400
Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> 2013/2/6 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> >> diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
> >> --- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
> >> +++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
> >> 15:44:09.869254397 +0800
> >> @@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
> >>          } while (rc == -EAGAIN);
> >>
> >>          for (i = 0; i < wdata->nr_pages; i++) {
> >> -                if (rc != 0)
> >> +                if (rc != 0){
> >>                          SetPageError(wdata->pages[i]);
> >> +                         end_page_writeback(wdata->pages[i]);
> >> +                         page_cache_release(wdata->pages[i]);
> >> +                 }
> >
> > I think we should do end_page_writeback(wdata->pages[i]) and
> > page_cache_release(wdata->pages[i]) regardless whether rc is 0 or not
> > (we are doing the same things in cifs_writev_complete now). Thoughts?
> >
> >>                  unlock_page(wdata->pages[i]);
> >>          }
> >>
> 
> Jeff, what do you think about my comments to this patch
> (http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commitdiff;h=c51bb0ea40ca038da26b1fa7d450f4078124af03)
> ? It doesn't clear to me why we should call
> end_page_writeback(wdata->pages[i]) and
> page_cache_release(wdata->pages[i]) in rc != 0 case only...
> 

It's a good question, so I'll go ahead and cc the mailing list since
there's no need to keep this private:

----------------[snip]---------------
        do {
                server = tlink_tcon(wdata->cfile->tlink)->ses->server;
                rc = server->ops->async_writev(wdata);
        } while (rc == -EAGAIN);

        for (i = 0; i < wdata->nr_pages; i++) {
                if (rc != 0)
                        SetPageError(wdata->pages[i]);
                unlock_page(wdata->pages[i]);
        }
----------------[snip]---------------

rc != 0 there means that resending the write failed with a
non-retryable error. In that case, we need to end the writeback on it
and release the reference we hold on it since the code is "finished"
with the page at that point. We don't expect a reply from the server,
so we need to make sure we clean up properly here.

If rc == 0 however, then there is now a new write request in progress.
The normal async reply handling will take care of it at that point. So
we need to continue to hold on to the page reference, and the writeback
bit still needs to stay set.

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

Re: [PATCH] cifs: bugfix for unreclaimed writeback pages in cifs_writev_requeue()

[Pavel Shilovsky]

2013/3/2 Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>:
> On Fri, 1 Mar 2013 22:08:04 +0400
> Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> 2013/2/6 Pavel Shilovsky <piastryyy-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> >> diff -uprN linux-3.8-rc6/fs/cifs/cifssmb.c linux-3.8-rc6_new/fs/cifs/cifssmb.c
>> >> --- linux-3.8-rc6/fs/cifs/cifssmb.c        2013-02-01 09:08:14.000000000 +0800
>> >> +++ linux-3.8-rc6_new/fs/cifs/cifssmb.c        2013-02-04
>> >> 15:44:09.869254397 +0800
>> >> @@ -1909,8 +1909,11 @@ cifs_writev_requeue(struct cifs_writedat
>> >>          } while (rc == -EAGAIN);
>> >>
>> >>          for (i = 0; i < wdata->nr_pages; i++) {
>> >> -                if (rc != 0)
>> >> +                if (rc != 0){
>> >>                          SetPageError(wdata->pages[i]);
>> >> +                         end_page_writeback(wdata->pages[i]);
>> >> +                         page_cache_release(wdata->pages[i]);
>> >> +                 }
>> >
>> > I think we should do end_page_writeback(wdata->pages[i]) and
>> > page_cache_release(wdata->pages[i]) regardless whether rc is 0 or not
>> > (we are doing the same things in cifs_writev_complete now). Thoughts?
>> >
>> >>                  unlock_page(wdata->pages[i]);
>> >>          }
>> >>
>>
>> Jeff, what do you think about my comments to this patch
>> (http://git.samba.org/?p=sfrench/cifs-2.6.git;a=commitdiff;h=c51bb0ea40ca038da26b1fa7d450f4078124af03)
>> ? It doesn't clear to me why we should call
>> end_page_writeback(wdata->pages[i]) and
>> page_cache_release(wdata->pages[i]) in rc != 0 case only...
>>
>
> It's a good question, so I'll go ahead and cc the mailing list since
> there's no need to keep this private:
>
> ----------------[snip]---------------
>         do {
>                 server = tlink_tcon(wdata->cfile->tlink)->ses->server;
>                 rc = server->ops->async_writev(wdata);
>         } while (rc == -EAGAIN);
>
>         for (i = 0; i < wdata->nr_pages; i++) {
>                 if (rc != 0)
>                         SetPageError(wdata->pages[i]);
>                 unlock_page(wdata->pages[i]);
>         }
> ----------------[snip]---------------
>
> rc != 0 there means that resending the write failed with a
> non-retryable error. In that case, we need to end the writeback on it
> and release the reference we hold on it since the code is "finished"
> with the page at that point. We don't expect a reply from the server,
> so we need to make sure we clean up properly here.
>
> If rc == 0 however, then there is now a new write request in progress.
> The normal async reply handling will take care of it at that point. So
> we need to continue to hold on to the page reference, and the writeback
> bit still needs to stay set.

Ah, thanks - now I understand why it's so. Sorry for that noise.

In this case, I agree with the patch. Steve, you can leave my
"reviewed-by" tag there.

-- 
Best regards,
Pavel Shilovsky.