On Mon, 1 Feb 2010, John Baldwin wrote:
I'd say that your patch works.
John, are you okay with that patch?
http://people.freebsd.org/~marius/fha_extract_info_realign2.diff
It's intention is to:
- Move nfs_realign() from the NFS client to the shared NFS code and
remove the NFS server version in order to reduce code duplication.
The shared version now uses a second parameter how, which is passed
on to m_get(9) and m_getcl(9) as the server used M_WAIT while the
client requires M_DONTWAIT, and replaces the the previously unused
parameter hsiz.
I don't think the client side can use M_WAIT/M_WAITOK if it wants to.
(I believe that it was once called from the socket upcall and that was
why M_DONTWAIT was used, but that isn't how it is called in the client
now.)
I mentioned this to dfr@ because I use a version shared between client
and server for the experimental one that does M_WAIT, but he didn't
think the regular client needed to be changed. Basically, I think the
current code in the regular client can return ENOMEM for I/O system calls,
which probably isn't what the app. expected. In the NFS client, you end up
with this "do I wait forever?" or "do I return an error to an I/O system
call which an app. doesn't expect" issue cropping up here and there. I
don't know the correct answer to this, but tend to lean in the "wait
forever" direction.
- Change nfs_realign() to use nfsm_aligned() so as with other NFS code
the alignment check isn't actually performed on platforms without
strict alignment requirements for performance reasons because as the
comment suggests only occasionally occurs with TCP.
- Change fha_extract_info() to use nfs_realign() with M_NOWAIT rather
than M_DONTWAIT because it's called with the RPC sp_lock held.
Btw, from an historical perspective, this was originally added for the
DEC PMAX port (MIPS R2000), which would trap whenever an unaligned ptr
was used. Then, there was this involved chunk of MIPS assembly code that
worked around the unaligned ptr access and the trap returned. Obviously
this was a major performance hit and happened fairly frequently for NFS
over ISO TP4. As you've seen, it happens infrequently enough over TCP
(back then, I think it was only when the TCP window size ended up really
small, although I'm way out of date on the TCP stack, so I have no idea
now:-) that I'd only do the re-alignment on achitectures that will crash
if it isn't done?
I missed the beginning of this. Was there crashes occurring because
the alignment wasn't being done or problems because the alignment
wasn't working correctly? (I guess I'm trying to say that, if the
arch works without nfs_realign(), then you might consider just not
doing it for that arch. I suppose that isn't a good enough reason
to not fix the function, though?;-)
The only downside of the shared nfs_realign() are the combined
SYSCTL counters but the fact we incremented them non-atomically
so far I think already indicates that their intention only is a
rough indication rather than exact values for client and server.
I'll admit I don't see how NFSCLIENT and NFSSERVER can both be loaded
as modules without the stuff in sys/nfs/nfs_common.c coming up
multiply defined, but it seems to work, so...
This all sounds good to me, but isn't M_NOWAIT == M_DONTWAIT? Hmm, reading
the code more closely, it looks like fha_extract_info() was using M_WAIT
rather than M_DONTWAIT previously. Also, I think you should be careful to use
M_DONTWAIT instead of M_NOWAIT for mbufs, so I would fix that in
fha_extract_info().
As noted above, I think the client can use M_WAIT safely. It was just a
design choice to do otherwise.
Have fun with it, rick
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-stable
To unsubscribe, send any mail to "[email protected]"
