Date:   Tue, 08 Sep 2020 12:10:46 -0400
From:   Jeff Layton <jlayton@...nel.org>
To:     Jan Kara <jack@...e.cz>,
        "Michael Kerrisk (man-pages)" <mtk.manpages@...il.com>
Cc:     milan.opensource@...il.com, lkml <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] fsync.2: ERRORS: add EIO and ENOSPC

On Tue, 2020-09-08 at 13:27 +0200, Jan Kara wrote:
> Added Jeff to CC since he has written the code...
> 
> On Mon 07-09-20 09:11:06, Michael Kerrisk (man-pages) wrote:
> > [Widening the CC to include Andrew and linux-fsdevel@]
> > [Milan: thanks for the patch, but it's unclear to me from your commit
> > message how/if you verified the details.]
> > 
> > Andrew, maybe you (or someone else) can comment, since long ago your
> > 
> >     commit f79e2abb9bd452d97295f34376dedbec9686b986
> >     Author: Andrew Morton <akpm@...l.org>
> >     Date:   Fri Mar 31 02:30:42 2006 -0800
> > 
> > included a comment that is referred to in  stackoverflow discussion
> > about this topic (that SO discussion is in turn referred to by
> > https://bugzilla.kernel.org/show_bug.cgi?id=194757).
> > 
> > The essence as I understand it, is this:
> > (1) fsync() (and similar) may fail EIO or ENOSPC, at which point data
> > has not been synced.
> > (2) In this case, the EIO/ENOSPC setting is cleared so that...
> > (3) A subsequent fsync() might return success, but...
> > (4) That doesn't mean that the data in (1) landed on the disk.
> 
> Correct.
> 
> > The proposed manual page patch below wants to document this, but I'd
> > be happy to have an FS-knowledgeable person comment before I apply.
> 
> Just a small comment below:
> 
> > On Sat, 29 Aug 2020 at 09:13, <milan.opensource@...il.com> wrote:
> > > From: Milan Shah <milan.opensource@...il.com>
> > > 
> > > This Fix addresses Bug 194757.
> > > Ref: https://bugzilla.kernel.org/show_bug.cgi?id=194757
> > > ---
> > >  man2/fsync.2 | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/man2/fsync.2 b/man2/fsync.2
> > > index 96401cd..f38b3e4 100644
> > > --- a/man2/fsync.2
> > > +++ b/man2/fsync.2
> > > @@ -186,6 +186,19 @@ In these cases disk caches need to be disabled using
> > >  or
> > >  .BR sdparm (8)
> > >  to guarantee safe operation.
> > > +
> > > +When
> > > +.BR fsync ()
> > > +or
> > > +.BR fdatasync ()
> > > +returns
> > > +.B EIO
> > > +or
> > > +.B ENOSPC
> > > +any error flags on pages in the file mapping are cleared, so subsequent synchronisation attempts
> > > +will return without error. It is
> > > +.I not
> > > +safe to retry synchronisation and assume that a non-error return means prior writes are now on disk.
> > >  .SH SEE ALSO
> > >  .BR sync (1),
> > >  .BR bdflush (2),
> 
> So the error state isn't really stored "on pages in the file mapping".
> Current implementation (since 4.14) is that error state is stored in struct
> file (I think this tends to be called "file description" in manpages) and
> so EIO / ENOSPC is reported once for each file description of the file that
> was open before the error happened. Not sure if we want to be so precise in
> the manpages or if it just confuses people. Anyway your takeway that no
> error on subsequent fsync() does not mean data was written is correct.
> 
> 								Honza
> 

Yep.

My only comment is that there is nothing special about EIO and ENOSPC.
All errors are the same in this regard. Basically, issuing a new fsync
after a failed one doesn't do any good. You need to redirty the pages
first.
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists