nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
info. In the event that fh_getattr fails, it resorts to scraping cached
values out of the inode directly.
Since these attributes are optional, we can just skip providing them
altogether when this happens.
Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/nfsfh.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)
diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
index ccd8485fee04..e8e13ae72e3c 100644
--- a/fs/nfsd/nfsfh.c
+++ b/fs/nfsd/nfsfh.c
@@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
inode = d_inode(fhp->fh_dentry);
err = fh_getattr(fhp, &stat);
- if (err) {
- /* Grab the times from inode anyway */
- stat.mtime = inode->i_mtime;
- stat.ctime = inode->i_ctime;
- stat.size = inode->i_size;
- if (v4 && IS_I_VERSION(inode)) {
- stat.change_cookie = inode_query_iversion(inode);
- stat.result_mask |= STATX_CHANGE_COOKIE;
- }
- }
+ if (err)
+ return;
+
if (v4)
fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
@@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
printk("nfsd: inode locked twice during operation.\n");
err = fh_getattr(fhp, &fhp->fh_post_attr);
- if (err) {
- fhp->fh_post_saved = false;
- fhp->fh_post_attr.ctime = inode->i_ctime;
- if (v4 && IS_I_VERSION(inode)) {
- fhp->fh_post_attr.change_cookie = inode_query_iversion(inode);
- fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;
- }
- } else
- fhp->fh_post_saved = true;
+ if (err)
+ return;
+
+ fhp->fh_post_saved = true;
if (v4)
fhp->fh_post_change =
nfsd4_change_attribute(&fhp->fh_post_attr, inode);
--
2.40.1
On Fri, 2023-05-19 at 07:17 -0400, Jeff Layton wrote:
> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
> info. In the event that fh_getattr fails, it resorts to scraping
> cached
> values out of the inode directly.
>
> Since these attributes are optional, we can just skip providing them
> altogether when this happens.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfsfh.c | 26 +++++++-------------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index ccd8485fee04..e8e13ae72e3c 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>
> inode = d_inode(fhp->fh_dentry);
> err = fh_getattr(fhp, &stat);
> - if (err) {
> - /* Grab the times from inode anyway */
> - stat.mtime = inode->i_mtime;
> - stat.ctime = inode->i_ctime;
> - stat.size = inode->i_size;
> - if (v4 && IS_I_VERSION(inode)) {
> - stat.change_cookie =
> inode_query_iversion(inode);
> - stat.result_mask |= STATX_CHANGE_COOKIE;
> - }
> - }
> + if (err)
> + return;
> +
> if (v4)
> fhp->fh_pre_change = nfsd4_change_attribute(&stat,
> inode);
>
> @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> printk("nfsd: inode locked twice during
> operation.\n");
>
> err = fh_getattr(fhp, &fhp->fh_post_attr);
> - if (err) {
> - fhp->fh_post_saved = false;
> - fhp->fh_post_attr.ctime = inode->i_ctime;
> - if (v4 && IS_I_VERSION(inode)) {
> - fhp->fh_post_attr.change_cookie =
> inode_query_iversion(inode);
> - fhp->fh_post_attr.result_mask |=
> STATX_CHANGE_COOKIE;
> - }
> - } else
> - fhp->fh_post_saved = true;
> + if (err)
> + return;
> +
> + fhp->fh_post_saved = true;
> if (v4)
> fhp->fh_post_change =
> nfsd4_change_attribute(&fhp->fh_post_attr,
> inode);
Unfortunately, I did recently find a corner case where this behaviour
will break the Linux NFSv3 client. In the case where READ sometimes
returns post-op attributes, and sometimes not, we can end up triggering
the 'out_overflow' in xdr_get_next_encode_buffer(), resulting in an EIO
error.
The problem is ultimately due to the attempt by the client to align the
pages to where it expects the READ reply to occur. When the behaviour
is unpredictable, then it sometimes ends up allocating too little
memory for the attributes, and ends up getting confused.
This bug does need to be fixed in the client, but just a warning that
the above server patch would be capable of triggering it.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Fri, 2023-05-19 at 13:08 +0000, Trond Myklebust wrote:
> On Fri, 2023-05-19 at 07:17 -0400, Jeff Layton wrote:
> > nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
> > info. In the event that fh_getattr fails, it resorts to scraping
> > cached
> > values out of the inode directly.
> >
> > Since these attributes are optional, we can just skip providing them
> > altogether when this happens.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > ?fs/nfsd/nfsfh.c | 26 +++++++-------------------
> > ?1 file changed, 7 insertions(+), 19 deletions(-)
> >
> > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > index ccd8485fee04..e8e13ae72e3c 100644
> > --- a/fs/nfsd/nfsfh.c
> > +++ b/fs/nfsd/nfsfh.c
> > @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > ?
> > ????????inode = d_inode(fhp->fh_dentry);
> > ????????err = fh_getattr(fhp, &stat);
> > -???????if (err) {
> > -???????????????/* Grab the times from inode anyway */
> > -???????????????stat.mtime = inode->i_mtime;
> > -???????????????stat.ctime = inode->i_ctime;
> > -???????????????stat.size? = inode->i_size;
> > -???????????????if (v4 && IS_I_VERSION(inode)) {
> > -???????????????????????stat.change_cookie =
> > inode_query_iversion(inode);
> > -???????????????????????stat.result_mask |= STATX_CHANGE_COOKIE;
> > -???????????????}
> > -???????}
> > +???????if (err)
> > +???????????????return;
> > +
> > ????????if (v4)
> > ????????????????fhp->fh_pre_change = nfsd4_change_attribute(&stat,
> > inode);
> > ?
> > @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> > ????????????????printk("nfsd: inode locked twice during
> > operation.\n");
> > ?
> > ????????err = fh_getattr(fhp, &fhp->fh_post_attr);
> > -???????if (err) {
> > -???????????????fhp->fh_post_saved = false;
> > -???????????????fhp->fh_post_attr.ctime = inode->i_ctime;
> > -???????????????if (v4 && IS_I_VERSION(inode)) {
> > -???????????????????????fhp->fh_post_attr.change_cookie =
> > inode_query_iversion(inode);
> > -???????????????????????fhp->fh_post_attr.result_mask |=
> > STATX_CHANGE_COOKIE;
> > -???????????????}
> > -???????} else
> > -???????????????fhp->fh_post_saved = true;
> > +???????if (err)
> > +???????????????return;
> > +
> > +???????fhp->fh_post_saved = true;
> > ????????if (v4)
> > ????????????????fhp->fh_post_change =
> > ????????????????????????nfsd4_change_attribute(&fhp->fh_post_attr,
> > inode);
>
> Unfortunately, I did recently find a corner case where this behaviour
> will break the Linux NFSv3 client. In the case where READ sometimes
> returns post-op attributes, and sometimes not, we can end up triggering
> the 'out_overflow' in xdr_get_next_encode_buffer(), resulting in an EIO
> error.
>
> The problem is ultimately due to the attempt by the client to align the
> pages to where it expects the READ reply to occur. When the behaviour
> is unpredictable, then it sometimes ends up allocating too little
> memory for the attributes, and ends up getting confused.
>
> This bug does need to be fixed in the client, but just a warning that
> the above server patch would be capable of triggering it.
>
Thanks for the heads up. This is not a critical issue, so I'm OK with
delaying this change if it's going to cause undue pain in the field. We
could also consider providing a module option or something in the
meantime, to give people a workaround if they hit it.
OTOH, this should only rarely happen. getattr doesn't often fail unless
you're exporting something like NFS or Ceph and someone does something
nefarious on the backend server/cluster.
--
Jeff Layton <[email protected]>
> On May 19, 2023, at 9:26 AM, Jeff Layton <[email protected]> wrote:
>
> On Fri, 2023-05-19 at 13:08 +0000, Trond Myklebust wrote:
>> On Fri, 2023-05-19 at 07:17 -0400, Jeff Layton wrote:
>>> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
>>> info. In the event that fh_getattr fails, it resorts to scraping
>>> cached
>>> values out of the inode directly.
>>>
>>> Since these attributes are optional, we can just skip providing them
>>> altogether when this happens.
>>>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> fs/nfsd/nfsfh.c | 26 +++++++-------------------
>>> 1 file changed, 7 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>> index ccd8485fee04..e8e13ae72e3c 100644
>>> --- a/fs/nfsd/nfsfh.c
>>> +++ b/fs/nfsd/nfsfh.c
>>> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>>>
>>> inode = d_inode(fhp->fh_dentry);
>>> err = fh_getattr(fhp, &stat);
>>> - if (err) {
>>> - /* Grab the times from inode anyway */
>>> - stat.mtime = inode->i_mtime;
>>> - stat.ctime = inode->i_ctime;
>>> - stat.size = inode->i_size;
>>> - if (v4 && IS_I_VERSION(inode)) {
>>> - stat.change_cookie =
>>> inode_query_iversion(inode);
>>> - stat.result_mask |= STATX_CHANGE_COOKIE;
>>> - }
>>> - }
>>> + if (err)
>>> + return;
>>> +
>>> if (v4)
>>> fhp->fh_pre_change = nfsd4_change_attribute(&stat,
>>> inode);
>>>
>>> @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
>>> printk("nfsd: inode locked twice during
>>> operation.\n");
>>>
>>> err = fh_getattr(fhp, &fhp->fh_post_attr);
>>> - if (err) {
>>> - fhp->fh_post_saved = false;
>>> - fhp->fh_post_attr.ctime = inode->i_ctime;
>>> - if (v4 && IS_I_VERSION(inode)) {
>>> - fhp->fh_post_attr.change_cookie =
>>> inode_query_iversion(inode);
>>> - fhp->fh_post_attr.result_mask |=
>>> STATX_CHANGE_COOKIE;
>>> - }
>>> - } else
>>> - fhp->fh_post_saved = true;
>>> + if (err)
>>> + return;
>>> +
>>> + fhp->fh_post_saved = true;
>>> if (v4)
>>> fhp->fh_post_change =
>>> nfsd4_change_attribute(&fhp->fh_post_attr,
>>> inode);
>>
>> Unfortunately, I did recently find a corner case where this behaviour
>> will break the Linux NFSv3 client. In the case where READ sometimes
>> returns post-op attributes, and sometimes not, we can end up triggering
>> the 'out_overflow' in xdr_get_next_encode_buffer(), resulting in an EIO
>> error.
>>
>> The problem is ultimately due to the attempt by the client to align the
>> pages to where it expects the READ reply to occur. When the behaviour
>> is unpredictable, then it sometimes ends up allocating too little
>> memory for the attributes, and ends up getting confused.
>>
>> This bug does need to be fixed in the client, but just a warning that
>> the above server patch would be capable of triggering it.
>>
>
> Thanks for the heads up. This is not a critical issue, so I'm OK with
> delaying this change if it's going to cause undue pain in the field. We
> could also consider providing a module option or something in the
> meantime, to give people a workaround if they hit it.
>
> OTOH, this should only rarely happen. getattr doesn't often fail unless
> you're exporting something like NFS or Ceph and someone does something
> nefarious on the backend server/cluster.
Right, we expect that the fh_getattr() operation will fail only very
rarely, usually due to a file getting removed or renamed over at the
wrong instant.
If you can provide a fix for the client for v6.5, IMO it would be a
low risk to apply both this patch and your fix at the same time.
--
Chuck Lever
On Fri, 19 May 2023, Jeff Layton wrote:
> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
> info. In the event that fh_getattr fails, it resorts to scraping cached
> values out of the inode directly.
>
> Since these attributes are optional, we can just skip providing them
> altogether when this happens.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfsfh.c | 26 +++++++-------------------
> 1 file changed, 7 insertions(+), 19 deletions(-)
>
> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> index ccd8485fee04..e8e13ae72e3c 100644
> --- a/fs/nfsd/nfsfh.c
> +++ b/fs/nfsd/nfsfh.c
> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>
> inode = d_inode(fhp->fh_dentry);
> err = fh_getattr(fhp, &stat);
> - if (err) {
> - /* Grab the times from inode anyway */
> - stat.mtime = inode->i_mtime;
> - stat.ctime = inode->i_ctime;
> - stat.size = inode->i_size;
> - if (v4 && IS_I_VERSION(inode)) {
> - stat.change_cookie = inode_query_iversion(inode);
> - stat.result_mask |= STATX_CHANGE_COOKIE;
> - }
> - }
> + if (err)
> + return;
> +
I wondered if this might exercise error paths which had not previously
been tested. Before this change fh_pre_saved is always set, now it is
not.
The code looks OK, but I was amused by xdr_stream_encode_item_absent().
Various places in the code test for "< 0" or "> 0" which seems to
suggest that "0" is not being handled consistently.
But of course xdr_stream_encode_item_absent() can never return 0. It
returns either XDR_UNIT or -EMSGSIZE.
I wonder if we should be consistent in how we test for an error .... or
if it it really matters.
Patch itself looks good.
Thanks,
NeilBrown
> if (v4)
> fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
>
> @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> printk("nfsd: inode locked twice during operation.\n");
>
> err = fh_getattr(fhp, &fhp->fh_post_attr);
> - if (err) {
> - fhp->fh_post_saved = false;
> - fhp->fh_post_attr.ctime = inode->i_ctime;
> - if (v4 && IS_I_VERSION(inode)) {
> - fhp->fh_post_attr.change_cookie = inode_query_iversion(inode);
> - fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;
> - }
> - } else
> - fhp->fh_post_saved = true;
> + if (err)
> + return;
> +
> + fhp->fh_post_saved = true;
> if (v4)
> fhp->fh_post_change =
> nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> --
> 2.40.1
>
>
> On May 21, 2023, at 9:24 PM, NeilBrown <[email protected]> wrote:
>
> On Fri, 19 May 2023, Jeff Layton wrote:
>> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
>> info. In the event that fh_getattr fails, it resorts to scraping cached
>> values out of the inode directly.
>>
>> Since these attributes are optional, we can just skip providing them
>> altogether when this happens.
>>
>> Signed-off-by: Jeff Layton <[email protected]>
>> ---
>> fs/nfsd/nfsfh.c | 26 +++++++-------------------
>> 1 file changed, 7 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>> index ccd8485fee04..e8e13ae72e3c 100644
>> --- a/fs/nfsd/nfsfh.c
>> +++ b/fs/nfsd/nfsfh.c
>> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>>
>> inode = d_inode(fhp->fh_dentry);
>> err = fh_getattr(fhp, &stat);
>> - if (err) {
>> - /* Grab the times from inode anyway */
>> - stat.mtime = inode->i_mtime;
>> - stat.ctime = inode->i_ctime;
>> - stat.size = inode->i_size;
>> - if (v4 && IS_I_VERSION(inode)) {
>> - stat.change_cookie = inode_query_iversion(inode);
>> - stat.result_mask |= STATX_CHANGE_COOKIE;
>> - }
>> - }
>> + if (err)
>> + return;
>> +
>
> I wondered if this might exercise error paths which had not previously
> been tested. Before this change fh_pre_saved is always set, now it is
> not.
>
> The code looks OK, but I was amused by xdr_stream_encode_item_absent().
> Various places in the code test for "< 0" or "> 0" which seems to
> suggest that "0" is not being handled consistently.
You can read those as "returns positive" and "returns negative" tests.
> But of course xdr_stream_encode_item_absent() can never return 0. It
> returns either XDR_UNIT or -EMSGSIZE.
I don't see any tests for it returning exactly zero.
> I wonder if we should be consistent in how we test for an error .... or
> if it it really matters.
The xdr_stream_encode_* functions conventionally return a negative errno
or a positive number of bytes encoded. The "< 0" and "> 0" tests convert
that return value into a boolean.
I reviewed the call sites just now and do not see an evident problem.
> Patch itself looks good.
May I add "Reviewed-by: Neil Brown <[email protected] <mailto:[email protected]>>" ?
> Thanks,
> NeilBrown
>
>
>> if (v4)
>> fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
>>
>> @@ -660,15 +653,10 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
>> printk("nfsd: inode locked twice during operation.\n");
>>
>> err = fh_getattr(fhp, &fhp->fh_post_attr);
>> - if (err) {
>> - fhp->fh_post_saved = false;
>> - fhp->fh_post_attr.ctime = inode->i_ctime;
>> - if (v4 && IS_I_VERSION(inode)) {
>> - fhp->fh_post_attr.change_cookie = inode_query_iversion(inode);
>> - fhp->fh_post_attr.result_mask |= STATX_CHANGE_COOKIE;
>> - }
>> - } else
>> - fhp->fh_post_saved = true;
>> + if (err)
>> + return;
>> +
>> + fhp->fh_post_saved = true;
>> if (v4)
>> fhp->fh_post_change =
>> nfsd4_change_attribute(&fhp->fh_post_attr, inode);
>> --
>> 2.40.1
--
Chuck Lever
On Tue, 23 May 2023, Chuck Lever III wrote:
>
> > On May 21, 2023, at 9:24 PM, NeilBrown <[email protected]> wrote:
> >
> > On Fri, 19 May 2023, Jeff Layton wrote:
> >> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
> >> info. In the event that fh_getattr fails, it resorts to scraping cached
> >> values out of the inode directly.
> >>
> >> Since these attributes are optional, we can just skip providing them
> >> altogether when this happens.
> >>
> >> Signed-off-by: Jeff Layton <[email protected]>
> >> ---
> >> fs/nfsd/nfsfh.c | 26 +++++++-------------------
> >> 1 file changed, 7 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> >> index ccd8485fee04..e8e13ae72e3c 100644
> >> --- a/fs/nfsd/nfsfh.c
> >> +++ b/fs/nfsd/nfsfh.c
> >> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> >>
> >> inode = d_inode(fhp->fh_dentry);
> >> err = fh_getattr(fhp, &stat);
> >> - if (err) {
> >> - /* Grab the times from inode anyway */
> >> - stat.mtime = inode->i_mtime;
> >> - stat.ctime = inode->i_ctime;
> >> - stat.size = inode->i_size;
> >> - if (v4 && IS_I_VERSION(inode)) {
> >> - stat.change_cookie = inode_query_iversion(inode);
> >> - stat.result_mask |= STATX_CHANGE_COOKIE;
> >> - }
> >> - }
> >> + if (err)
> >> + return;
> >> +
> >
> > I wondered if this might exercise error paths which had not previously
> > been tested. Before this change fh_pre_saved is always set, now it is
> > not.
> >
> > The code looks OK, but I was amused by xdr_stream_encode_item_absent().
> > Various places in the code test for "< 0" or "> 0" which seems to
> > suggest that "0" is not being handled consistently.
>
> You can read those as "returns positive" and "returns negative" tests.
That leaves the curious reader, who isn't completely familiar with the
code, wondering what "0" would mean.
It's not a big deal, but it looked odd so I thought I would mention it.
>
>
> > But of course xdr_stream_encode_item_absent() can never return 0. It
> > returns either XDR_UNIT or -EMSGSIZE.
>
> I don't see any tests for it returning exactly zero.
>
>
> > I wonder if we should be consistent in how we test for an error .... or
> > if it it really matters.
>
> The xdr_stream_encode_* functions conventionally return a negative errno
> or a positive number of bytes encoded. The "< 0" and "> 0" tests convert
> that return value into a boolean.
>
> I reviewed the call sites just now and do not see an evident problem.
>
>
> > Patch itself looks good.
>
> May I add "Reviewed-by: Neil Brown <[email protected] <mailto:[email protected]>>" ?
Yes please. (though maybe without the "mailto:" :-)
NeilBrown
> On May 23, 2023, at 6:03 PM, NeilBrown <[email protected]> wrote:
>
> On Tue, 23 May 2023, Chuck Lever III wrote:
>>
>>> On May 21, 2023, at 9:24 PM, NeilBrown <[email protected]> wrote:
>>>
>>> On Fri, 19 May 2023, Jeff Layton wrote:
>>>> nfsd calls fh_getattr to get the latest inode attrs for pre/post-op
>>>> info. In the event that fh_getattr fails, it resorts to scraping cached
>>>> values out of the inode directly.
>>>>
>>>> Since these attributes are optional, we can just skip providing them
>>>> altogether when this happens.
>>>>
>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>> ---
>>>> fs/nfsd/nfsfh.c | 26 +++++++-------------------
>>>> 1 file changed, 7 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
>>>> index ccd8485fee04..e8e13ae72e3c 100644
>>>> --- a/fs/nfsd/nfsfh.c
>>>> +++ b/fs/nfsd/nfsfh.c
>>>> @@ -623,16 +623,9 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
>>>>
>>>> inode = d_inode(fhp->fh_dentry);
>>>> err = fh_getattr(fhp, &stat);
>>>> - if (err) {
>>>> - /* Grab the times from inode anyway */
>>>> - stat.mtime = inode->i_mtime;
>>>> - stat.ctime = inode->i_ctime;
>>>> - stat.size = inode->i_size;
>>>> - if (v4 && IS_I_VERSION(inode)) {
>>>> - stat.change_cookie = inode_query_iversion(inode);
>>>> - stat.result_mask |= STATX_CHANGE_COOKIE;
>>>> - }
>>>> - }
>>>> + if (err)
>>>> + return;
>>>> +
>>>
>>> I wondered if this might exercise error paths which had not previously
>>> been tested. Before this change fh_pre_saved is always set, now it is
>>> not.
>>>
>>> The code looks OK, but I was amused by xdr_stream_encode_item_absent().
>>> Various places in the code test for "< 0" or "> 0" which seems to
>>> suggest that "0" is not being handled consistently.
>>
>> You can read those as "returns positive" and "returns negative" tests.
>
> That leaves the curious reader, who isn't completely familiar with the
> code, wondering what "0" would mean.
> It's not a big deal, but it looked odd so I thought I would mention it.
Code readability is always on point.
The return values are a feature of all the xdr_stream_encode_* helpers.
For _item_absent() in particular, we could go with "!= XDR_UNIT" but
that's more verbose and still not especially more clear.
Probably the one spot that is confusing is the "_item_absent() > 0"
call site. That's meant to mean "true if it worked, false if not".
There was again no real better alternative.
>>> But of course xdr_stream_encode_item_absent() can never return 0. It
>>> returns either XDR_UNIT or -EMSGSIZE.
>>
>> I don't see any tests for it returning exactly zero.
>>
>>
>>> I wonder if we should be consistent in how we test for an error .... or
>>> if it it really matters.
>>
>> The xdr_stream_encode_* functions conventionally return a negative errno
>> or a positive number of bytes encoded. The "< 0" and "> 0" tests convert
>> that return value into a boolean.
>>
>> I reviewed the call sites just now and do not see an evident problem.
>>
>>
>>> Patch itself looks good.
>>
>> May I add "Reviewed-by: Neil Brown <[email protected] <mailto:[email protected]>>" ?
>
> Yes please. (though maybe without the "mailto:" :-)
Thanks.
I typed the address correctly, but oddly, Mail.app helpfully added the
mailto: tag when it sent the mail. Ho hum.
--
Chuck Lever