On Sat, 22 Jun 2024, Mike Snitzer wrote:
> On Fri, Jun 21, 2024 at 04:08:20PM +1000, NeilBrown wrote:
> >
> > Both branches of this if() do exactly the same thing. iov_iter_advance
> > is a no-op if the size arg is zero.
>
> iov_iter_advance doesn't look to be a no-op if the size arg is zero.
void iov_iter_advance(struct iov_iter *i, size_t size)
{
if (unlikely(i->count < size))
size = i->count;
if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {
i->iov_offset += size;
i->count -= size;
} else if (likely(iter_is_iovec(i) || iov_iter_is_kvec(i))) {
/* iovec and kvec have identical layouts */
iov_iter_iovec_advance(i, size);
} else if (iov_iter_is_bvec(i)) {
iov_iter_bvec_advance(i, size);
} else if (iov_iter_is_discard(i)) {
i->count -= size;
}
}
This adds "size" to offset, and subtracts "size" from count. For iovec
and bvec it is a slightly complicated dance to achieve this, but that is
the net result.
So if "size" is zero there is no change to the iov_iter. Just some
wasted cycles. Do those cycles justify the extra conditional branch? I
don't know. I would generally prefer simpler code which is only
optimised with evidence. Admittedly I don't always follow that
preference myself and I won't hold you to it. But I thought the review
would be incomplete without mentioning it.
>
> > Is it really worth increasing the code size to sometimes avoid a
> > function call?
> >
> > At least we should for the iov_iter_bvec() inconditionally, then maybe
> > call _advance().
>
> For v7, I've fixed it so we do what you suggest.
Thanks.
> > > +static void
> > > +nfs_get_vfs_attr(struct file *filp, struct nfs_fattr *fattr)
> > > +{
> > > + struct kstat stat;
> > > +
> > > + if (fattr != NULL && vfs_getattr(&filp->f_path, &stat,
> > > + STATX_INO |
> > > + STATX_ATIME |
> > > + STATX_MTIME |
> > > + STATX_CTIME |
> > > + STATX_SIZE |
> > > + STATX_BLOCKS,
> > > + AT_STATX_SYNC_AS_STAT) == 0) {
> > > + fattr->valid = NFS_ATTR_FATTR_FILEID |
> > > + NFS_ATTR_FATTR_CHANGE |
> > > + NFS_ATTR_FATTR_SIZE |
> > > + NFS_ATTR_FATTR_ATIME |
> > > + NFS_ATTR_FATTR_MTIME |
> > > + NFS_ATTR_FATTR_CTIME |
> > > + NFS_ATTR_FATTR_SPACE_USED;
> > > + fattr->fileid = stat.ino;
> > > + fattr->size = stat.size;
> > > + fattr->atime = stat.atime;
> > > + fattr->mtime = stat.mtime;
> > > + fattr->ctime = stat.ctime;
> > > + fattr->change_attr = nfs_timespec_to_change_attr(&fattr->ctime);
> >
> > This looks wrong for NFSv4. I think we should use
> > nfsd4_change_attribute().
> > Maybe it isn't important, but if it isn't I'd like to see an explanation
> > why.
> >
> > > + fattr->du.nfs3.used = stat.blocks << 9;
> > > + }
> > > +}
>
> Not following, this is client code so it doesn't have access to
> nfsd4_change_attribute().
This is nfs-localio code which blurs the boundary between server and
client...
The change_attr is used by NFS to detect if a file might have changed.
This code is used to get the attributes after a write request.
NFS uses a GETATTR request to the server at other times. The
change_attr should be consistent between the two else comparisons will
be meaningless.
So I think that nfs_get_vfs_attr() should use the same change_attr as
the one that would be used if the NFS GETATTR request were made.
For NFSv3, that is nfs_timespec_to_change_attr() as you have.
For NFSv4 it is something different.
I think that having inconsistent change_attrs could cause NFS to flush
its page cache unnecessarily. As it can read directly from the
server-side where is likely is cached, that might not be a problem. If
that reasoning does apply it should be explained.
However there is talk of exporting the "i_version" number to userspace
through xattr. For NFS that is essentially the change_attr. If we did
that we would really want to keep the number consistent.
We could easily move nfsd4_change_attribute() into nfs_common or even
make it an inline in some common include file. It doesn't use any nfsd
internals.
Thanks,
NeilBrown
>
> Pending clarification, and further review on my part, leaving this
> item to one side (so won't be addressed in v7).
>
> Thanks,
> Mike
>
