Date: Sat, 13 Jul 1996 11:47:02 +0100 From: Doug Rabson <dfr@nlsys.demon.co.uk> To: current@freebsd.org Cc: dfr@render.com Subject: NFSv3 fixes for review Message-ID: <31E77EA6.41C67EA6@nlsys.demon.co.uk>
next in thread | raw e-mail | index | archive | help
I have just integrated some fixes to the NFSv3 code from Frank van der
Linden (frank@fwi.uva.nl). The first bug is the same one as the
'corrupt output from makesyscalls.sh on an nfsv3 mount' bug. I have
verified this one by testing on a loopback mount (which is all I can
do from my home machine). I have not tried to reproduce the second
two problems but the fixes seem safe.
While I was here, I also fixed the truncated 32bit minor numbers on
nfsv3 mount bug. I believe that Bruce originally suggested the fix
for this?
Could someone please review these changes (diffs included) so that I
can commit them.
P.S. Is there a policy on using remote cvs to freefall? I don't have
a cvs tree at home, so I used remote cvs to checkout the nfs source
and produce diffs. What about commits via remote cvs?
Date: Fri, 12 Jan 1996 02:17:04 +0100 (MET)
From: frank@fwi.uva.nl (Frank van der Linden)
Subject: Re: nfs v3 bug
Thanks for the suggestions on the bug I reported. Too bad you don't have
a machine around there anymore on which you run the code (well.. unless
you get that VaxStation port going ;-))
Anyway, I think I found the bug. The scenario is as follows:
- a process opens a file over NFS for writing, using v3
- at some point it calls nfs_write(), which partially fills a buffer
with bdwrite
- an nfsiod grabs it, because it was doing an operation anyway,
and it checks if more buffers are available
- nfssvc_iod calls nfs_doio, and the buffer gets written to
the server, but not committed to the server. The buffer
will now have B_DELWRI and B_NEEDCOMMIT set
- the process does another nfs_write, resulting in another
bdwrite() into the same buffer.
- there is now a buffer which still has B_DELWRI and
B_NEEDCOMMIT set, and thus is looked at as being
'written to the server but not committed'. This is
wrong, because the buffer now contains more data than
was originally written to the server.
- when nfs_flush() is called, the buffer is only committed,
the extra data never makes it to the server.
The fix I applied was simply clearing B_NEEDCOMMIT in bdwrite(). Perhaps
this was already done on the systems (BSDI, 4.4BSD?) you tested the code
on.
- Frank
----
Date: Mon, 1 Jul 1996 11:42:05 +0200 (MET DST)
From: frank@fwi.uva.nl (Frank van der Linden)
Subject: NFSv3 bugfix
Hi,
Here's an NFSv3 code bugfix I thought you should know about. The bug was,
that whenever a server (for whatever reason) returned an answer to a
write RPC that implied that fewer bytes than requested were written, bad
things would happen.
Following happened:
- nfs_writerpc sees that not all data was written
- it backs up the counter and pointer in the iov struct part of the uiop
- the write loop is entered again
- nfsm_uiotom is entered again (i.e. nfsm_uiotombuf())
- the first time that was called, uio_iov was incremented, because
the iov had been completely 'emptied' into mbufs, so normally you'd
move on to the next. however, iovcnt is always 1 in this case
--> uio_iov is a bogus pointer now, pointing somewhere on the stack
of nfs_doio(), where a local iov was used in the uio structure
- bad things happen when the bogus pointer is referenced.
Fix: make sure that iovcnt is always 1, and don't increment uio_iov, since
this should never happen. I also added a comment noting that nfsm_uiotombuf()
can only handle iovcnt == 1.
The fix is appended at the end of this mail.
---
Date: Wed, 10 Jul 1996 11:08:55 +0200 (MET DST)
From: frank@fwi.uva.nl (Frank van der Linden)
Subject: Some new fixes
Hi,
I know that you don't have the time currently to look at it, but i thought
I'd mail you these patches for the NFSv3 code anyway. There were 2 problems:
1) The setattr operation passed on the atime in stead of the mtime to
the server. The fix is trivial.
2) XIDs always started at 0, but this caused some servers (older DEC OSF/1 3.0
so I've been told) who had very long-lasting XID caches to get confused
if, after a reboot of a BSD client, RPCs came in with a XID that had
in the past been used before from that client. Patch is to use the
current time in seconds as a starting point for XIDs. The patch below
is not perfect, because it requires the root fs to be mounted first. This
is because of the check BSD systems do, comparing FS time to system time.
The patches are appended below.
- Frank
Index: nfs_bio.c
===================================================================
RCS file: /home/ncvs/src/sys/nfs/nfs_bio.c,v
retrieving revision 1.23
diff -c -r1.23 nfs_bio.c
*** nfs_bio.c 1996/06/08 05:59:04 1.23
--- nfs_bio.c 1996/07/13 10:35:53
***************
*** 584,589 ****
--- 584,596 ----
bp->b_validoff = min(bp->b_validoff, bp->b_dirtyoff);
bp->b_validend = max(bp->b_validend, bp->b_dirtyend);
}
+
+ /*
+ * Since this block is being modified, it must be written
+ * again and not just committed.
+ */
+ bp->b_flags &= ~B_NEEDCOMMIT;
+
/*
* If the lease is non-cachable or IO_SYNC do bwrite().
*/
Index: nfs_subs.c
===================================================================
RCS file: /home/ncvs/src/sys/nfs/nfs_subs.c,v
retrieving revision 1.30
diff -c -r1.30 nfs_subs.c
*** nfs_subs.c 1996/06/23 17:19:25 1.30
--- nfs_subs.c 1996/07/13 10:37:48
***************
*** 53,58 ****
--- 53,59 ----
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/malloc.h>
+ #include <sys/time.h>
#ifdef VFS_LKM
#include <sys/sysent.h>
#include <sys/syscall.h>
***************
*** 635,640 ****
--- 636,643 ----
register int i;
struct mbuf *mreq, *mb2;
int siz, grpsiz, authsiz;
+ struct timeval tv;
+ static u_long base;
authsiz = nfsm_rndup(auth_len);
MGETHDR(mb, M_WAIT, MT_DATA);
***************
*** 653,660 ****
--- 656,677 ----
* First the RPC header.
*/
nfsm_build(tl, u_long *, 8 * NFSX_UNSIGNED);
+
+ /*
+ * derive initial xid from system time
+ * XXX time is invalid if root not yet mounted
+ */
+ if (!base && (rootvp)) {
+ microtime(&tv);
+ base = tv.tv_sec << 12;
+ nfs_xid = base;
+ }
+ /*
+ * Skip zero xid if it should ever happen.
+ */
if (++nfs_xid == 0)
nfs_xid++;
+
*tl++ = *xidp = txdr_unsigned(nfs_xid);
*tl++ = rpc_call;
*tl++ = rpc_vers;
***************
*** 834,840 ****
}
/*
! * copies a uio scatter/gather list to an mbuf chain...
*/
int
nfsm_uiotombuf(uiop, mq, siz, bpos)
--- 851,858 ----
}
/*
! * copies a uio scatter/gather list to an mbuf chain.
! * NOTE: can ony handle iovcnt == 1
*/
int
nfsm_uiotombuf(uiop, mq, siz, bpos)
***************
*** 849,854 ****
--- 867,877 ----
int uiosiz, clflg, rem;
char *cp;
+ #ifdef DIAGNOSTIC
+ if (uiop->uio_iovcnt != 1)
+ panic("nfsm_uiotombuf: iovcnt != 1");
+ #endif
+
if (siz > MLEN) /* or should it >= MCLBYTES ?? */
clflg = 1;
else
***************
*** 856,863 ****
rem = nfsm_rndup(siz)-siz;
mp = mp2 = *mq;
while (siz > 0) {
- if (uiop->uio_iovcnt <= 0 || uiop->uio_iov == NULL)
- return (EINVAL);
left = uiop->uio_iov->iov_len;
uiocp = uiop->uio_iov->iov_base;
if (left > siz)
--- 879,884 ----
***************
*** 892,904 ****
uiop->uio_offset += xfer;
uiop->uio_resid -= xfer;
}
! if (uiop->uio_iov->iov_len <= siz) {
! uiop->uio_iovcnt--;
! uiop->uio_iov++;
! } else {
! uiop->uio_iov->iov_base += uiosiz;
! uiop->uio_iov->iov_len -= uiosiz;
! }
siz -= uiosiz;
}
if (rem > 0) {
--- 913,920 ----
uiop->uio_offset += xfer;
uiop->uio_resid -= xfer;
}
! uiop->uio_iov->iov_base += uiosiz;
! uiop->uio_iov->iov_len -= uiosiz;
siz -= uiosiz;
}
if (rem > 0) {
***************
*** 1218,1225 ****
if (v3) {
vtyp = nfsv3tov_type(fp->fa_type);
vmode = fxdr_unsigned(u_short, fp->fa_mode);
! rdev = makedev(fxdr_unsigned(int, fp->fa3_rdev.specdata1),
! fxdr_unsigned(int, fp->fa3_rdev.specdata2));
fxdr_nfsv3time(&fp->fa3_mtime, &mtime);
} else {
vtyp = nfsv2tov_type(fp->fa_type);
--- 1234,1241 ----
if (v3) {
vtyp = nfsv3tov_type(fp->fa_type);
vmode = fxdr_unsigned(u_short, fp->fa_mode);
! rdev = makedev(fxdr_unsigned(u_int, fp->fa3_rdev.specdata1),
! fxdr_unsigned(u_int, fp->fa3_rdev.specdata2));
fxdr_nfsv3time(&fp->fa3_mtime, &mtime);
} else {
vtyp = nfsv2tov_type(fp->fa_type);
Index: nfs_vnops.c
===================================================================
RCS file: /home/ncvs/src/sys/nfs/nfs_vnops.c,v
retrieving revision 1.33
diff -c -r1.33 nfs_vnops.c
*** nfs_vnops.c 1996/01/25 00:45:37 1.33
--- nfs_vnops.c 1996/07/13 10:40:38
***************
*** 757,763 ****
if (vap->va_mtime.ts_sec != time.tv_sec) {
nfsm_build(tl, u_long *, 3 * NFSX_UNSIGNED);
*tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT);
! txdr_nfsv3time(&vap->va_atime, tl);
} else {
nfsm_build(tl, u_long *, NFSX_UNSIGNED);
*tl = txdr_unsigned(NFSV3SATTRTIME_TOSERVER);
--- 757,763 ----
if (vap->va_mtime.ts_sec != time.tv_sec) {
nfsm_build(tl, u_long *, 3 * NFSX_UNSIGNED);
*tl++ = txdr_unsigned(NFSV3SATTRTIME_TOCLIENT);
! txdr_nfsv3time(&vap->va_mtime, tl);
} else {
nfsm_build(tl, u_long *, NFSX_UNSIGNED);
*tl = txdr_unsigned(NFSV3SATTRTIME_TOSERVER);
--
Doug Rabson Mail: dfr@nlsys.demon.co.uk
Phone: +44 181 951 1891
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?31E77EA6.41C67EA6>