[PATCH] nfs: commit layouts in fdatasync

[PATCH] nfs: commit layouts in fdatasync

[Christoph Hellwig]

>From fdatasync(2):

 "fdatasync() is similar to fsync(), but does not flush modified metadata
  unless that metadata is needed in order  to  allow  a  subsequent  data
  retrieval to be correctly handled."

We absolutely need to commit the layouts to be able to retrieve the data
in case either the client, the server or the storage subsystem go down.

Signed-off-by: Christoph Hellwig <hch@lst.de>

diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
index 8de3407..464db9d 100644
--- a/fs/nfs/nfs4file.c
+++ b/fs/nfs/nfs4file.c
@@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 			break;
 		mutex_lock(&inode->i_mutex);
 		ret = nfs_file_fsync_commit(file, start, end, datasync);
-		if (!ret && !datasync)
-			/* application has asked for meta-data sync */
+		if (!ret)
 			ret = pnfs_layoutcommit_inode(inode, true);
 		mutex_unlock(&inode->i_mutex);
 		/*

Re: [PATCH] nfs: commit layouts in fdatasync

[Christoph Hellwig]

ping?  This is a fairly serious data integrity issue for pnfs users.

On Mon, Apr 21, 2014 at 10:29:17AM -0700, Christoph Hellwig wrote:
> >From fdatasync(2):
> 
>  "fdatasync() is similar to fsync(), but does not flush modified metadata
>   unless that metadata is needed in order  to  allow  a  subsequent  data
>   retrieval to be correctly handled."
> 
> We absolutely need to commit the layouts to be able to retrieve the data
> in case either the client, the server or the storage subsystem go down.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> index 8de3407..464db9d 100644
> --- a/fs/nfs/nfs4file.c
> +++ b/fs/nfs/nfs4file.c
> @@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  			break;
>  		mutex_lock(&inode->i_mutex);
>  		ret = nfs_file_fsync_commit(file, start, end, datasync);
> -		if (!ret && !datasync)
> -			/* application has asked for meta-data sync */
> +		if (!ret)
>  			ret = pnfs_layoutcommit_inode(inode, true);
>  		mutex_unlock(&inode->i_mutex);
>  		/*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH] nfs: commit layouts in fdatasync

[Christoph Hellwig]

On Sun, May 04, 2014 at 11:56:24PM -0700, Christoph Hellwig wrote:
> ping?  This is a fairly serious data integrity issue for pnfs users.

ping^2

without this fdatasync and O_DSYNC writes on pnfs are more or less
noops, so at least getting a review would be helpful, nevermind
forwarding it to Linus and -stable.

> 
> On Mon, Apr 21, 2014 at 10:29:17AM -0700, Christoph Hellwig wrote:
> > >From fdatasync(2):
> > 
> >  "fdatasync() is similar to fsync(), but does not flush modified metadata
> >   unless that metadata is needed in order  to  allow  a  subsequent  data
> >   retrieval to be correctly handled."
> > 
> > We absolutely need to commit the layouts to be able to retrieve the data
> > in case either the client, the server or the storage subsystem go down.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
> > index 8de3407..464db9d 100644
> > --- a/fs/nfs/nfs4file.c
> > +++ b/fs/nfs/nfs4file.c
> > @@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> >  			break;
> >  		mutex_lock(&inode->i_mutex);
> >  		ret = nfs_file_fsync_commit(file, start, end, datasync);
> > -		if (!ret && !datasync)
> > -			/* application has asked for meta-data sync */
> > +		if (!ret)
> >  			ret = pnfs_layoutcommit_inode(inode, true);
> >  		mutex_unlock(&inode->i_mutex);
> >  		/*
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> ---end quoted text---
---end quoted text---

Re: [PATCH] nfs: commit layouts in fdatasync

[Trond Myklebust]

On Tue, May 27, 2014 at 12:00 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sun, May 04, 2014 at 11:56:24PM -0700, Christoph Hellwig wrote:
>> ping?  This is a fairly serious data integrity issue for pnfs users.
>
> ping^2
>
> without this fdatasync and O_DSYNC writes on pnfs are more or less
> noops, so at least getting a review would be helpful, nevermind
> forwarding it to Linus and -stable.

Applied for 3.16. Not sure about -stable eligibility, since this only
affects pnfs blocks for now (files and objects should both be able to
recover in case of client failure). Are you seeing this in production
environments?

Cheers
  Trond

>> On Mon, Apr 21, 2014 at 10:29:17AM -0700, Christoph Hellwig wrote:
>> > >From fdatasync(2):
>> >
>> >  "fdatasync() is similar to fsync(), but does not flush modified metadata
>> >   unless that metadata is needed in order  to  allow  a  subsequent  data
>> >   retrieval to be correctly handled."
>> >
>> > We absolutely need to commit the layouts to be able to retrieve the data
>> > in case either the client, the server or the storage subsystem go down.
>> >
>> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> >
>> > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c
>> > index 8de3407..464db9d 100644
>> > --- a/fs/nfs/nfs4file.c
>> > +++ b/fs/nfs/nfs4file.c
>> > @@ -100,8 +100,7 @@ nfs4_file_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>> >                     break;
>> >             mutex_lock(&inode->i_mutex);
>> >             ret = nfs_file_fsync_commit(file, start, end, datasync);
>> > -           if (!ret && !datasync)
>> > -                   /* application has asked for meta-data sync */
>> > +           if (!ret)
>> >                     ret = pnfs_layoutcommit_inode(inode, true);
>> >             mutex_unlock(&inode->i_mutex);
>> >             /*
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> ---end quoted text---
> ---end quoted text---



-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: [PATCH] nfs: commit layouts in fdatasync

[Christoph Hellwig]

On Wed, May 28, 2014 at 07:00:08PM -0400, Trond Myklebust wrote:
> Applied for 3.16. Not sure about -stable eligibility, since this only
> affects pnfs blocks for now (files and objects should both be able to
> recover in case of client failure).

At least the linux-nfs.org object server needs the layoutcommit to
update i_size on the inode, so changes won't be persistent without the
layoutcommit.  I expect loosely coupled file servers to work in a
similar fashion.

> Are you seeing this in production
> environments?

This was found during data integrity testing.