All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Hans-Peter Jansen" <hpj-2x7n3sizJbFeoWH0uzbU5w@public.gmane.org>
To: linux-kernel@vger.kernel.org
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>,
	Aaron Straus <aaron-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>,
	Chuck Lever <chuck.lever@oracle.com>, Neil Brown <neilb@suse.de>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [NFS] blocks of zeros (NULLs) in NFS files in kernels >= 2.6.20
Date: Mon, 22 Sep 2008 20:45:44 +0200	[thread overview]
Message-ID: <200809222045.45990.hpj@urpla.net> (raw)
In-Reply-To: <1222101322.7615.6.camel@localhost>

Am Montag, 22. September 2008 schrieb Trond Myklebust:
> On Mon, 2008-09-22 at 18:05 +0200, Hans-Peter Jansen wrote:
> > For what is worth, this behavior is visible in bog standard
> > writing/reading files, (log files in my case, via the python logging
> > package). It obviously deviates from local filesystem behavior, and
> > former state of the linux nfs-client. Should we add patches to less,
> > tail, and all other instruments for watching/analysing log files (just
> > to pick the tip of the ice rock) in order to throw away runs of zeros,
> > when reading from nfs mounted files? Or should we ask their maintainers
> > to add locking code for the nfs "read files, which are written at the
> > same time" case, just to work around __some__ of the consequences of
> > this bug? Imagine, how ugly this is going to look!
> >
> > The whole issue is what I call a major regression, thus I strongly ask
> > for a reply from Trond on this matter.
> >
> > I even vote for sending a revert request for this hunk to the stable
> > team, where it is applicable, after Trond sorted it out (for 2.6.27?).
> >
> > Thanks, Aaron and Chuck for the detailed analysis - it demystified a
> > wired behavior, I observed here. When you're in a process to get real
> > work done in a fixed timeline, such things could make you mad..
>
> Revert _what_ exactly?
For your convenience, important parts inlined here:

>From Aarons message: Tue, 9 Sep 2008 12:46:44 -0700 in this thread. << EOM

Of the bisected offending commit:

commit e261f51f25b98c213e0b3d7f2109b117d714f69d
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Tue Dec 5 00:35:41 2006 -0500

    NFS: Make nfs_updatepage() mark the page as dirty.
    
    This will ensure that we can call set_page_writeback() from within
    nfs_writepage(), which is always called with the page lock set.
    
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>


It seems to be this hunk which introduces the problem:


@@ -628,7 +667,6 @@ static struct nfs_page * nfs_update_request(struct 
nfs_open_context* ctx,
                                return ERR_PTR(error);
                        }
                        spin_unlock(&nfsi->req_lock);
-                       nfs_mark_request_dirty(new);
                        return new;
                }
                spin_unlock(&nfsi->req_lock);


If I add that function call back in... the problem disappears.  I don't
know if this just papers over the real problem though?  

EOM

This commit happened between 2.6.19 and 2.6.20, btw.

> Please assume that I've been travelling for the past 5 weeks, and have
> only a sketchy idea of what has been going on.

Ahh, I see, that explains, why you didn't responded earlier.

> My understanding was that this is a consequence of unordered writes
> causing the file to be extended while some other task is reading.
> AFAICS, this sort of behaviour has _always_ been possible. I can't see
> how reverting anything will fix it.

Hopefully, this helps you to remember the purpose of that change.

Cheers,
Pete


WARNING: multiple messages have this Message-ID (diff)
From: "Hans-Peter Jansen" <hpj@urpla.net>
To: linux-kernel@vger.kernel.org
Cc: Trond Myklebust <trond.myklebust@fys.uio.no>,
	Aaron Straus <aaron@merfinllc.com>,
	Chuck Lever <chuck.lever@oracle.com>, Neil Brown <neilb@suse.de>,
	Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [NFS] blocks of zeros (NULLs) in NFS files in kernels >= 2.6.20
Date: Mon, 22 Sep 2008 20:45:44 +0200	[thread overview]
Message-ID: <200809222045.45990.hpj@urpla.net> (raw)
In-Reply-To: <1222101322.7615.6.camel@localhost>

Am Montag, 22. September 2008 schrieb Trond Myklebust:
> On Mon, 2008-09-22 at 18:05 +0200, Hans-Peter Jansen wrote:
> > For what is worth, this behavior is visible in bog standard
> > writing/reading files, (log files in my case, via the python logging
> > package). It obviously deviates from local filesystem behavior, and
> > former state of the linux nfs-client. Should we add patches to less,
> > tail, and all other instruments for watching/analysing log files (just
> > to pick the tip of the ice rock) in order to throw away runs of zeros,
> > when reading from nfs mounted files? Or should we ask their maintainers
> > to add locking code for the nfs "read files, which are written at the
> > same time" case, just to work around __some__ of the consequences of
> > this bug? Imagine, how ugly this is going to look!
> >
> > The whole issue is what I call a major regression, thus I strongly ask
> > for a reply from Trond on this matter.
> >
> > I even vote for sending a revert request for this hunk to the stable
> > team, where it is applicable, after Trond sorted it out (for 2.6.27?).
> >
> > Thanks, Aaron and Chuck for the detailed analysis - it demystified a
> > wired behavior, I observed here. When you're in a process to get real
> > work done in a fixed timeline, such things could make you mad..
>
> Revert _what_ exactly?
For your convenience, important parts inlined here:

>From Aarons message: Tue, 9 Sep 2008 12:46:44 -0700 in this thread. << EOM

Of the bisected offending commit:

commit e261f51f25b98c213e0b3d7f2109b117d714f69d
Author: Trond Myklebust <Trond.Myklebust@netapp.com>
Date:   Tue Dec 5 00:35:41 2006 -0500

    NFS: Make nfs_updatepage() mark the page as dirty.
    
    This will ensure that we can call set_page_writeback() from within
    nfs_writepage(), which is always called with the page lock set.
    
    Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>


It seems to be this hunk which introduces the problem:


@@ -628,7 +667,6 @@ static struct nfs_page * nfs_update_request(struct 
nfs_open_context* ctx,
                                return ERR_PTR(error);
                        }
                        spin_unlock(&nfsi->req_lock);
-                       nfs_mark_request_dirty(new);
                        return new;
                }
                spin_unlock(&nfsi->req_lock);


If I add that function call back in... the problem disappears.  I don't
know if this just papers over the real problem though?  

EOM

This commit happened between 2.6.19 and 2.6.20, btw.

> Please assume that I've been travelling for the past 5 weeks, and have
> only a sketchy idea of what has been going on.

Ahh, I see, that explains, why you didn't responded earlier.

> My understanding was that this is a consequence of unordered writes
> causing the file to be extended while some other task is reading.
> AFAICS, this sort of behaviour has _always_ been possible. I can't see
> how reverting anything will fix it.

Hopefully, this helps you to remember the purpose of that change.

Cheers,
Pete


  parent reply	other threads:[~2008-09-22 18:45 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-09-05 19:19 [NFS] blocks of zeros (NULLs) in NFS files in kernels >= 2.6.20 Aaron Straus
2008-09-05 19:19 ` Aaron Straus
     [not found] ` <20080905191939.GG22796-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-05 19:56   ` [NFS] " Chuck Lever
2008-09-05 19:56     ` Chuck Lever
2008-09-05 20:04     ` Aaron Straus
2008-09-05 20:36       ` Bernd Eckenfels
     [not found]       ` <20080905200455.GH22796-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-05 20:36         ` Chuck Lever
2008-09-05 20:36           ` Chuck Lever
2008-09-05 22:14           ` Aaron Straus
2008-09-05 22:14             ` Aaron Straus
2008-09-06  0:03     ` Aaron Straus
2008-09-06  0:03       ` Aaron Straus
2008-09-08 19:02     ` Aaron Straus
2008-09-08 19:02       ` Aaron Straus
     [not found]       ` <20080908190212.GF28123-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-08 21:15         ` Chuck Lever
2008-09-08 21:15           ` Chuck Lever
     [not found]           ` <76bd70e30809081415h6b55a8dfl8171634c576ac946-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-09-08 22:02             ` Aaron Straus
2008-09-08 22:02               ` Aaron Straus
2008-09-09 19:46             ` Aaron Straus
2008-09-09 19:46               ` Aaron Straus
     [not found]               ` <20080909194644.GI5290-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-11 16:55                 ` Chuck Lever
2008-09-11 16:55                   ` Chuck Lever
2008-09-11 17:19                   ` Aaron Straus
2008-09-11 17:19                     ` Aaron Straus
     [not found]                     ` <20080911171908.GE12037-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-11 17:48                       ` Chuck Lever
2008-09-11 17:48                         ` Chuck Lever
2008-09-11 18:49                         ` Aaron Straus
2008-09-11 18:49                           ` Aaron Straus
     [not found]                           ` <20080911184951.GB19054-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-22 16:05                             ` Hans-Peter Jansen
2008-09-22 16:05                               ` Hans-Peter Jansen
     [not found]                               ` <200809221805.48463.hpj-2x7n3sizJbFeoWH0uzbU5w@public.gmane.org>
2008-09-22 16:35                                 ` Trond Myklebust
2008-09-22 16:35                                   ` Trond Myklebust
2008-09-22 17:04                                   ` Aaron Straus
2008-09-22 17:04                                     ` Aaron Straus
     [not found]                                     ` <20080922170414.GC12483-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-22 17:26                                       ` Chuck Lever
2008-09-22 17:26                                         ` Chuck Lever
     [not found]                                         ` <76bd70e30809221026g7bde774pbffa35881682ea4b-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2008-09-22 17:37                                           ` Aaron Straus
2008-09-22 17:37                                             ` Aaron Straus
2008-09-22 17:29                                       ` Trond Myklebust
2008-09-22 17:29                                         ` Trond Myklebust
2008-09-22 17:45                                         ` Aaron Straus
2008-09-22 17:45                                           ` Aaron Straus
     [not found]                                           ` <20080922174525.GF12483-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org>
2008-09-22 18:43                                             ` Aaron Straus
2008-09-22 18:43                                               ` Aaron Straus
2008-09-22 18:45                                             ` Hans-Peter Jansen
2008-09-22 18:45                                               ` Hans-Peter Jansen
2008-09-22 18:45                                   ` Hans-Peter Jansen [this message]
2008-09-22 18:45                                     ` Hans-Peter Jansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200809222045.45990.hpj@urpla.net \
    --to=hpj-2x7n3sizjbfeowh0uzbu5w@public.gmane.org \
    --cc=aaron-bYFJunmd+ZV8UrSeD/g0lQ@public.gmane.org \
    --cc=chuck.lever@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    --cc=trond.myklebust@fys.uio.no \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.