[PATCH 1/5] NFSv4/pnfs: Ensure we don't miss a file extension

[PATCH 1/5] NFSv4/pnfs: Ensure we don't miss a file extension

[Trond Myklebust]

pNFS writes don't return attributes, however that doesn't mean that we
should ignore the fact that they may be extending the file. This patch
ensures that if a write is seen to extend the file, then we always set
an attribute barrier, and update the cached file size.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/write.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/write.c b/fs/nfs/write.c
index 359e9ad596c9..0e6a2b8786b4 100644
--- a/fs/nfs/write.c
+++ b/fs/nfs/write.c
@@ -1378,24 +1378,27 @@ static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr,
 {
 	struct nfs_pgio_args *argp = &hdr->args;
 	struct nfs_pgio_res *resp = &hdr->res;
+	u64 size = argp->offset + resp->count;
 
 	if (!(fattr->valid & NFS_ATTR_FATTR_SIZE))
+		fattr->size = size;
+	if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode)) {
+		fattr->valid &= ~NFS_ATTR_FATTR_SIZE;
 		return;
-	if (argp->offset + resp->count != fattr->size)
-		return;
-	if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode))
+	}
+	if (size != fattr->size)
 		return;
 	/* Set attribute barrier */
 	nfs_fattr_set_barrier(fattr);
+	/* ...and update size */
+	fattr->valid |= NFS_ATTR_FATTR_SIZE;
 }
 
 void nfs_writeback_update_inode(struct nfs_pgio_header *hdr)
 {
-	struct nfs_fattr *fattr = hdr->res.fattr;
+	struct nfs_fattr *fattr = &hdr->fattr;
 	struct inode *inode = hdr->inode;
 
-	if (fattr == NULL)
-		return;
 	spin_lock(&inode->i_lock);
 	nfs_writeback_check_extend(hdr, fattr);
 	nfs_post_op_update_inode_force_wcc_locked(inode, fattr);
-- 
2.4.3

[PATCH 2/5] NFS: Don't revalidate the mapping if both size and change attr are up to date

[Trond Myklebust]

If we've ensured that the size and the change attribute are both correct,
then there is no point in marking those attributes as needing revalidation
again. Only do so if we know the size is incorrect and was not updated.

Fixes: f2467b6f64da ("NFS: Clear NFS_INO_REVAL_PAGECACHE when...")
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index b77b328a06d7..d654661defb3 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -1244,9 +1244,11 @@ static int nfs_check_inode_attributes(struct inode *inode, struct nfs_fattr *fat
 	if (fattr->valid & NFS_ATTR_FATTR_SIZE) {
 		cur_size = i_size_read(inode);
 		new_isize = nfs_size_to_loff_t(fattr->size);
-		if (cur_size != new_isize && nfsi->nrequests == 0)
+		if (cur_size != new_isize)
 			invalid |= NFS_INO_INVALID_ATTR|NFS_INO_REVAL_PAGECACHE;
 	}
+	if (nfsi->nrequests != 0)
+		invalid &= ~NFS_INO_REVAL_PAGECACHE;
 
 	/* Have any file permissions changed? */
 	if ((fattr->valid & NFS_ATTR_FATTR_MODE) && (inode->i_mode & S_IALLUGO) != (fattr->mode & S_IALLUGO))
@@ -1684,8 +1686,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			invalid |= NFS_INO_INVALID_ATTR
 				| NFS_INO_INVALID_DATA
 				| NFS_INO_INVALID_ACCESS
-				| NFS_INO_INVALID_ACL
-				| NFS_INO_REVAL_PAGECACHE;
+				| NFS_INO_INVALID_ACL;
 			if (S_ISDIR(inode->i_mode))
 				nfs_force_lookup_revalidate(inode);
 			inode->i_version = fattr->change_attr;
@@ -1717,7 +1718,6 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 			if ((nfsi->nrequests == 0) || new_isize > cur_isize) {
 				i_size_write(inode, new_isize);
 				invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_DATA;
-				invalid &= ~NFS_INO_REVAL_PAGECACHE;
 			}
 			dprintk("NFS: isize change on server for file %s/%ld "
 					"(%Ld to %Ld)\n",
-- 
2.4.3

[PATCH 3/5] NFS: Set NFS_INO_REVAL_PAGECACHE if the change attribute is uninitialised

[Trond Myklebust]

We can't allow caching of data until the change attribute has been
initialised correctly.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index d654661defb3..426e4f8207ef 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -443,7 +443,8 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, st
 		if (fattr->valid & NFS_ATTR_FATTR_CHANGE)
 			inode->i_version = fattr->change_attr;
 		else if (nfs_server_capable(inode, NFS_CAP_CHANGE_ATTR))
-			nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR);
+			nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR
+				| NFS_INO_REVAL_PAGECACHE);
 		if (fattr->valid & NFS_ATTR_FATTR_SIZE)
 			inode->i_size = nfs_size_to_loff_t(fattr->size);
 		else
-- 
2.4.3

[PATCH 4/5] NFS: Remove the "NFS_CAP_CHANGE_ATTR" capability

[Trond Myklebust]

Setting the change attribute has been mandatory for all NFS versions, since
commit 3a1556e8662c ("NFSv2/v3: Simulate the change attribute"). We should
therefore not have anything be conditional on it being set/unset.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/client.c           | 2 +-
 fs/nfs/inode.c            | 4 ++--
 fs/nfs/nfs4proc.c         | 3 ---
 include/linux/nfs_fs_sb.h | 2 +-
 4 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index ecebb406cc1a..4a90c9bb3135 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -775,7 +775,7 @@ static int nfs_init_server(struct nfs_server *server,
 	server->options = data->options;
 	server->caps |= NFS_CAP_HARDLINKS|NFS_CAP_SYMLINKS|NFS_CAP_FILEID|
 		NFS_CAP_MODE|NFS_CAP_NLINK|NFS_CAP_OWNER|NFS_CAP_OWNER_GROUP|
-		NFS_CAP_ATIME|NFS_CAP_CTIME|NFS_CAP_MTIME|NFS_CAP_CHANGE_ATTR;
+		NFS_CAP_ATIME|NFS_CAP_CTIME|NFS_CAP_MTIME;
 
 	if (data->rsize)
 		server->rsize = nfs_block_size(data->rsize, NULL);
diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 426e4f8207ef..0adc7d245b3d 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -442,7 +442,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, st
 			nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR);
 		if (fattr->valid & NFS_ATTR_FATTR_CHANGE)
 			inode->i_version = fattr->change_attr;
-		else if (nfs_server_capable(inode, NFS_CAP_CHANGE_ATTR))
+		else
 			nfs_set_cache_invalid(inode, NFS_INO_INVALID_ATTR
 				| NFS_INO_REVAL_PAGECACHE);
 		if (fattr->valid & NFS_ATTR_FATTR_SIZE)
@@ -1692,7 +1692,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 				nfs_force_lookup_revalidate(inode);
 			inode->i_version = fattr->change_attr;
 		}
-	} else if (server->caps & NFS_CAP_CHANGE_ATTR)
+	} else
 		nfsi->cache_validity |= save_cache_validity;
 
 	if (fattr->valid & NFS_ATTR_FATTR_MTIME) {
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 9264994ec9d3..c85ffe67b5f3 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -8591,7 +8591,6 @@ static const struct nfs4_minor_version_ops nfs_v4_0_minor_ops = {
 	.minor_version = 0,
 	.init_caps = NFS_CAP_READDIRPLUS
 		| NFS_CAP_ATOMIC_OPEN
-		| NFS_CAP_CHANGE_ATTR
 		| NFS_CAP_POSIX_LOCK,
 	.init_client = nfs40_init_client,
 	.shutdown_client = nfs40_shutdown_client,
@@ -8617,7 +8616,6 @@ static const struct nfs4_minor_version_ops nfs_v4_1_minor_ops = {
 	.minor_version = 1,
 	.init_caps = NFS_CAP_READDIRPLUS
 		| NFS_CAP_ATOMIC_OPEN
-		| NFS_CAP_CHANGE_ATTR
 		| NFS_CAP_POSIX_LOCK
 		| NFS_CAP_STATEID_NFSV41
 		| NFS_CAP_ATOMIC_OPEN_V1,
@@ -8640,7 +8638,6 @@ static const struct nfs4_minor_version_ops nfs_v4_2_minor_ops = {
 	.minor_version = 2,
 	.init_caps = NFS_CAP_READDIRPLUS
 		| NFS_CAP_ATOMIC_OPEN
-		| NFS_CAP_CHANGE_ATTR
 		| NFS_CAP_POSIX_LOCK
 		| NFS_CAP_STATEID_NFSV41
 		| NFS_CAP_ATOMIC_OPEN_V1
diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
index a2ea1491d3df..20bc8e51b161 100644
--- a/include/linux/nfs_fs_sb.h
+++ b/include/linux/nfs_fs_sb.h
@@ -220,7 +220,7 @@ struct nfs_server {
 #define NFS_CAP_SYMLINKS	(1U << 2)
 #define NFS_CAP_ACLS		(1U << 3)
 #define NFS_CAP_ATOMIC_OPEN	(1U << 4)
-#define NFS_CAP_CHANGE_ATTR	(1U << 5)
+/* #define NFS_CAP_CHANGE_ATTR	(1U << 5) */
 #define NFS_CAP_FILEID		(1U << 6)
 #define NFS_CAP_MODE		(1U << 7)
 #define NFS_CAP_NLINK		(1U << 8)
-- 
2.4.3

[PATCH 5/5] NFS: nfs_mark_for_revalidate should always set NFS_INO_REVAL_PAGECACHE

[Trond Myklebust]

I'm not aware of any existing bugs around this, but the expectation is
that nfs_mark_for_revalidate() should always force a revalidation of
the cached metadata.

Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 include/linux/nfs_fs.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/linux/nfs_fs.h b/include/linux/nfs_fs.h
index f91b5ade30c9..874b77228fb9 100644
--- a/include/linux/nfs_fs.h
+++ b/include/linux/nfs_fs.h
@@ -292,9 +292,12 @@ static inline void nfs_mark_for_revalidate(struct inode *inode)
 	struct nfs_inode *nfsi = NFS_I(inode);
 
 	spin_lock(&inode->i_lock);
-	nfsi->cache_validity |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS;
+	nfsi->cache_validity |= NFS_INO_INVALID_ATTR |
+				NFS_INO_REVAL_PAGECACHE |
+				NFS_INO_INVALID_ACCESS |
+				NFS_INO_INVALID_ACL;
 	if (S_ISDIR(inode->i_mode))
-		nfsi->cache_validity |= NFS_INO_REVAL_PAGECACHE|NFS_INO_INVALID_DATA;
+		nfsi->cache_validity |= NFS_INO_INVALID_DATA;
 	spin_unlock(&inode->i_lock);
 }
 
-- 
2.4.3

Re: [PATCH 1/5] NFSv4/pnfs: Ensure we don't miss a file extension

[Peng Tao]

Hi Trond,

Can we please add following patch to stable as well? It not only fixes
pnfs write, but also fixes in band writes when delegations are
enabled. Without it, I can always get fstest read_write/mctime.t
failure with in band IO.

Thanks,
Tao

On Thu, Jul 23, 2015 at 11:48 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
> pNFS writes don't return attributes, however that doesn't mean that we
> should ignore the fact that they may be extending the file. This patch
> ensures that if a write is seen to extend the file, then we always set
> an attribute barrier, and update the cached file size.
>
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
>  fs/nfs/write.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
> index 359e9ad596c9..0e6a2b8786b4 100644
> --- a/fs/nfs/write.c
> +++ b/fs/nfs/write.c
> @@ -1378,24 +1378,27 @@ static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr,
>  {
>         struct nfs_pgio_args *argp = &hdr->args;
>         struct nfs_pgio_res *resp = &hdr->res;
> +       u64 size = argp->offset + resp->count;
>
>         if (!(fattr->valid & NFS_ATTR_FATTR_SIZE))
> +               fattr->size = size;
> +       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode)) {
> +               fattr->valid &= ~NFS_ATTR_FATTR_SIZE;
>                 return;
> -       if (argp->offset + resp->count != fattr->size)
> -               return;
> -       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode))
> +       }
> +       if (size != fattr->size)
>                 return;
>         /* Set attribute barrier */
>         nfs_fattr_set_barrier(fattr);
> +       /* ...and update size */
> +       fattr->valid |= NFS_ATTR_FATTR_SIZE;
>  }
>
>  void nfs_writeback_update_inode(struct nfs_pgio_header *hdr)
>  {
> -       struct nfs_fattr *fattr = hdr->res.fattr;
> +       struct nfs_fattr *fattr = &hdr->fattr;
>         struct inode *inode = hdr->inode;
>
> -       if (fattr == NULL)
> -               return;
>         spin_lock(&inode->i_lock);
>         nfs_writeback_check_extend(hdr, fattr);
>         nfs_post_op_update_inode_force_wcc_locked(inode, fattr);
> --
> 2.4.3
>

Re: [PATCH 1/5] NFSv4/pnfs: Ensure we don't miss a file extension

[Peng Tao]

On Wed, Aug 19, 2015 at 10:50 AM, Peng Tao <bergwolf@primarydata.com> wrote:
> Hi Trond,
>
> Can we please add following patch to stable as well? It not only fixes
> pnfs write, but also fixes in band writes when delegations are
> enabled. Without it, I can always get fstest read_write/mctime.t
> failure with in band IO.
ah, nvm, speaking too soon. I can still reproduce fstest
read_write/mctime.t failure on delegation+inband IO with your testing
branch...

Looking into it...

sorry for the noise.

>
> Thanks,
> Tao
>
> On Thu, Jul 23, 2015 at 11:48 AM, Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
>> pNFS writes don't return attributes, however that doesn't mean that we
>> should ignore the fact that they may be extending the file. This patch
>> ensures that if a write is seen to extend the file, then we always set
>> an attribute barrier, and update the cached file size.
>>
>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>>  fs/nfs/write.c | 15 +++++++++------
>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>> index 359e9ad596c9..0e6a2b8786b4 100644
>> --- a/fs/nfs/write.c
>> +++ b/fs/nfs/write.c
>> @@ -1378,24 +1378,27 @@ static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr,
>>  {
>>         struct nfs_pgio_args *argp = &hdr->args;
>>         struct nfs_pgio_res *resp = &hdr->res;
>> +       u64 size = argp->offset + resp->count;
>>
>>         if (!(fattr->valid & NFS_ATTR_FATTR_SIZE))
>> +               fattr->size = size;
>> +       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode)) {
>> +               fattr->valid &= ~NFS_ATTR_FATTR_SIZE;
>>                 return;
>> -       if (argp->offset + resp->count != fattr->size)
>> -               return;
>> -       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode))
>> +       }
>> +       if (size != fattr->size)
>>                 return;
>>         /* Set attribute barrier */
>>         nfs_fattr_set_barrier(fattr);
>> +       /* ...and update size */
>> +       fattr->valid |= NFS_ATTR_FATTR_SIZE;
>>  }
>>
>>  void nfs_writeback_update_inode(struct nfs_pgio_header *hdr)
>>  {
>> -       struct nfs_fattr *fattr = hdr->res.fattr;
>> +       struct nfs_fattr *fattr = &hdr->fattr;
>>         struct inode *inode = hdr->inode;
>>
>> -       if (fattr == NULL)
>> -               return;
>>         spin_lock(&inode->i_lock);
>>         nfs_writeback_check_extend(hdr, fattr);
>>         nfs_post_op_update_inode_force_wcc_locked(inode, fattr);
>> --
>> 2.4.3
>>

Re: [PATCH 1/5] NFSv4/pnfs: Ensure we don't miss a file extension

[Peng Tao]

On Wed, Aug 19, 2015 at 11:40 AM, Peng Tao <bergwolf@primarydata.com> wrote:
> On Wed, Aug 19, 2015 at 10:50 AM, Peng Tao <bergwolf@primarydata.com> wrote:
>> Hi Trond,
>>
>> Can we please add following patch to stable as well? It not only fixes
>> pnfs write, but also fixes in band writes when delegations are
>> enabled. Without it, I can always get fstest read_write/mctime.t
>> failure with in band IO.
> ah, nvm, speaking too soon. I can still reproduce fstest
> read_write/mctime.t failure on delegation+inband IO with your testing
> branch...
>
> Looking into it...
>
oh, man, we do need the commit to fix delegation+inband IO. My VM's
time got screwed up and my last tests did not include the commit...
sorry again for more noise :(...

> sorry for the noise.
>
>>
>> Thanks,
>> Tao
>>
>> On Thu, Jul 23, 2015 at 11:48 AM, Trond Myklebust
>> <trond.myklebust@primarydata.com> wrote:
>>> pNFS writes don't return attributes, however that doesn't mean that we
>>> should ignore the fact that they may be extending the file. This patch
>>> ensures that if a write is seen to extend the file, then we always set
>>> an attribute barrier, and update the cached file size.
>>>
>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>> ---
>>>  fs/nfs/write.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/nfs/write.c b/fs/nfs/write.c
>>> index 359e9ad596c9..0e6a2b8786b4 100644
>>> --- a/fs/nfs/write.c
>>> +++ b/fs/nfs/write.c
>>> @@ -1378,24 +1378,27 @@ static void nfs_writeback_check_extend(struct nfs_pgio_header *hdr,
>>>  {
>>>         struct nfs_pgio_args *argp = &hdr->args;
>>>         struct nfs_pgio_res *resp = &hdr->res;
>>> +       u64 size = argp->offset + resp->count;
>>>
>>>         if (!(fattr->valid & NFS_ATTR_FATTR_SIZE))
>>> +               fattr->size = size;
>>> +       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode)) {
>>> +               fattr->valid &= ~NFS_ATTR_FATTR_SIZE;
>>>                 return;
>>> -       if (argp->offset + resp->count != fattr->size)
>>> -               return;
>>> -       if (nfs_size_to_loff_t(fattr->size) < i_size_read(hdr->inode))
>>> +       }
>>> +       if (size != fattr->size)
>>>                 return;
>>>         /* Set attribute barrier */
>>>         nfs_fattr_set_barrier(fattr);
>>> +       /* ...and update size */
>>> +       fattr->valid |= NFS_ATTR_FATTR_SIZE;
>>>  }
>>>
>>>  void nfs_writeback_update_inode(struct nfs_pgio_header *hdr)
>>>  {
>>> -       struct nfs_fattr *fattr = hdr->res.fattr;
>>> +       struct nfs_fattr *fattr = &hdr->fattr;
>>>         struct inode *inode = hdr->inode;
>>>
>>> -       if (fattr == NULL)
>>> -               return;
>>>         spin_lock(&inode->i_lock);
>>>         nfs_writeback_check_extend(hdr, fattr);
>>>         nfs_post_op_update_inode_force_wcc_locked(inode, fattr);
>>> --
>>> 2.4.3
>>>