[nfs-discuss] code review for 6778894 - NEW fix
Hi Pavel, On Thu, Dec 18, 2008 at 10:54:41PM +0100, Pavel Filipensky wrote: >> Is there any recent push to the file not reflected yet in the public gate at >> opensolaris.org? >> > > Yes, the recent push is: > 16-Dec-2008 Jerry Gilliam 6783351 boot archive isn't updated > after patch install via installupdates smf service. Ok. Thanks. > > looks like src.opensolaris.org has a problem to display latest change, but > it knows about the change, see > http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/initpkg/umountall.sh?r=8385 Yes. You are true. It is already reported: http://defect.opensolaris.org/bz/show_bug.cgi?id=5557 Best regards. -- Marcel Telka Solaris RPE
[nfs-discuss] code review for 6778894 - NEW fix
On 12/18/08 19:17, Frank Batschulat (Home) wrote: > On Thu, 18 Dec 2008 14:45:28 +0100, Pavel Filipensky sun.com> wrote: > > >> Hi, >> >> to avoid confusion with outdated webrevs - the latest webrev is here >> >> http://cr.opensolaris.org/~pavelf/6778894-v3 >> > > Thanks to Nico, I'd say this is safe to go now :) > > fwiw, it'll be interesting how much positive influence this fix has to > http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6544130 > It should fix it. I see in 6544130 that svc.startd already contains the fix for 6675447 NFSv4 client hangs on shutdown if server is down beforehand, since -l is used for /sbin/umountall: R 3224 3212 7 7 0 0x4200 ff01a99b8318 /sbin/sh /sbin/umountall -l It means that no nfs filesystems should be accessed after fix for 6778894 is delivered and no hang should happen. I will do also explicit test for that. --Pavel > I'll perform my tests with this fix over christmas... > > > frankB > ___ > nfs-discuss mailing list > nfs-discuss at opensolaris.org >
[nfs-discuss] code review for 6778894 - NEW fix
>> http://cr.opensolaris.org/~pavelf/6778894-v3 >> > > It looks correct and I am ok with the change. > > Only very minor nit: The webrev is trying to say that a year at line 23 is > already 2008, but here: > http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/initpkg/umountall.sh > we could see that the year is still 2007 in the gate. > > Is there any recent push to the file not reflected yet in the public gate at > opensolaris.org? > Yes, the recent push is: 16-Dec-2008 Jerry Gilliam 6783351 boot archive isn't updated after patch install via installupdates smf service. looks like src.opensolaris.org has a problem to display latest change, but it knows about the change, see http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/initpkg/umountall.sh?r=8385 Thanks, Pavel
[nfs-discuss] code review for 6778894 - NEW fix
On Thu, 18 Dec 2008 14:45:28 +0100, Pavel Filipensky wrote: > Hi, > > to avoid confusion with outdated webrevs - the latest webrev is here > > http://cr.opensolaris.org/~pavelf/6778894-v3 Thanks to Nico, I'd say this is safe to go now :) fwiw, it'll be interesting how much positive influence this fix has to http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6544130 I'll perform my tests with this fix over christmas... frankB
[nfs-discuss] code review for 6778894 - NEW fix
Hi Pavel, On Wed, Dec 17, 2008 at 06:31:05PM +0100, Pavel Filipensky wrote: > I have updated the comments, new webrev is here (it also contains the > latest umountall changeset from today): > > http://cr.opensolaris.org/~pavelf/6778894-v3 It looks correct and I am ok with the change. Only very minor nit: The webrev is trying to say that a year at line 23 is already 2008, but here: http://src.opensolaris.org/source/xref/onnv/onnv-gate/usr/src/cmd/initpkg/umountall.sh we could see that the year is still 2007 in the gate. Is there any recent push to the file not reflected yet in the public gate at opensolaris.org? Have a nice day -- Marcel Telka Solaris RPE
[nfs-discuss] code review for 6778894 - NEW fix
On Thu, Dec 18, 2008 at 11:20:09PM +0100, Pavel Filipensky wrote: > On 12/18/08 19:17, Frank Batschulat (Home) wrote: > >fwiw, it'll be interesting how much positive influence this fix has to > >http://bugs.opensolaris.org/bugdatabase/view_bug.do?bug_id=6544130 > > It should fix it. I see in 6544130 that svc.startd already contains > the fix for 6675447 NFSv4 client hangs on shutdown if server is down > beforehand, since -l is used for /sbin/umountall: > > R 3224 3212 7 7 0 0x4200 ff01a99b8318 /sbin/sh /sbin/umountall -l > > It means that no nfs filesystems should be accessed after fix for > 6778894 is delivered and no hang should happen. I will do also > explicit test for that. And, needless to say (but for the benefit of other readers here), close one CR as a dup of the other.
[nfs-discuss] code review for 6778894 - NEW fix
Hi, to avoid confusion with outdated webrevs - the latest webrev is here http://cr.opensolaris.org/~pavelf/6778894-v3 the review was finished by Nico. Since then, I have done 2 minor changes: * renamed NFS_LIST to nfslist (to keep the naming consistent) * re-phrased the WARNING (removed the CR number) --Pavel Pavel Filipensky wrote: > Nicolas Williams wrote: > >> On Wed, Dec 17, 2008 at 06:31:05PM +0100, Pavel Filipensky wrote: >> >>> I have updated the comments, new webrev is here (it also contains the >>> latest umountall changeset from today): >>> >>> http://cr.opensolaris.org/~pavelf/6778894-v3 >>> >>> Can you give an explicit review of this workspace? >>> >> You need to explicitly set NFS_LIST= the empty string, otherwise if >> NFS_LIST happens to be set in the environment when unmountall runs... >> > > Thanks for catching this. I have added: > 246 NFS_LIST="" > > webrev is updated http://cr.opensolaris.org/~pavelf/6778894-v3 > > --Pavel > > > >> I think I have a better way to deal with the whitespace issues too, >> including newlines. I'll send a reply on the other thread sometime >> after lunch. >> > > ___ > nfs-discuss mailing list > nfs-discuss at opensolaris.org >
[nfs-discuss] code review for 6778894 - NEW fix
Nicolas Williams wrote: > On Wed, Dec 17, 2008 at 06:31:05PM +0100, Pavel Filipensky wrote: >> I have updated the comments, new webrev is here (it also contains the >> latest umountall changeset from today): >> >> http://cr.opensolaris.org/~pavelf/6778894-v3 >> >> Can you give an explicit review of this workspace? > > You need to explicitly set NFS_LIST= the empty string, otherwise if > NFS_LIST happens to be set in the environment when unmountall runs... Thanks for catching this. I have added: 246 NFS_LIST="" webrev is updated http://cr.opensolaris.org/~pavelf/6778894-v3 --Pavel > > I think I have a better way to deal with the whitespace issues too, > including newlines. I'll send a reply on the other thread sometime > after lunch.
[nfs-discuss] code review for 6778894 - NEW fix
I have updated the comments, new webrev is here (it also contains the latest umountall changeset from today): http://cr.opensolaris.org/~pavelf/6778894-v3 Can you give an explicit review of this workspace? Thanks, Pavel Frank Batschulat (Home) wrote: > On Wed, 17 Dec 2008 15:13:40 +0100, Pavel Filipensky sun.com> wrote: > > >> Two notes: >> >> 1) >> I am wondering if I should add this comment as a warning for future >> code changes >> which would go OTW and cause regression of "6675447 NFSv4 client hangs >> on shutdown if server is down beforehand." >> >> # we cannot us 'df -F nfs $mountp' to test if the given mountpoint has >> NFS on top of it >> # since df could trigger NFS over-the-wire request and cause regression of >> # "6675447 NFSv4 client hangs on shutdown if server is down beforehand." >> > > yeah I really do think we should put a big fat warning inthere, > that the use of any syscall on a NFS filesystem has the danger to go OTW > and thus defeat the whole purpose of what we're trying to achive right now. > > >> 2) >> The more I am looking to umountall(1M) the more I think that umountall(1M) >> should get a new syscall and all the work apart from parsing the command >> line options should be done in the kernel. >> >> Now, the kernel exports its state via a special file system in /etc/mnttab. >> Information from /etc/mnttab is parsed as a text in umountall(1M) - parsing >> is error prone - space can do lot of harm - see 6786256. After the >> information is parsed >> it is processed with low efficiency just to get a list of mount points >> to be unmounted. >> This list goes back to the kernel via umount(1). Current shell >> implementation is slow and hard to maintain. >> > > this has been annoying me for a long time, we do have the in kernel mnttab > and we do have the inkernel sharetab ! whats the story about this ancient > shell script > anyways these days ! > > I think we definitively should at least file a place holder RFE that we > have a record of this would be a good idea to re-architect. > > --- > frankB >
[nfs-discuss] code review for 6778894 - NEW fix
On Wed, 17 Dec 2008 15:13:40 +0100, Pavel Filipensky wrote: > Two notes: > > 1) > I am wondering if I should add this comment as a warning for future > code changes > which would go OTW and cause regression of "6675447 NFSv4 client hangs > on shutdown if server is down beforehand." > > # we cannot us 'df -F nfs $mountp' to test if the given mountpoint has > NFS on top of it > # since df could trigger NFS over-the-wire request and cause regression of > # "6675447 NFSv4 client hangs on shutdown if server is down beforehand." yeah I really do think we should put a big fat warning inthere, that the use of any syscall on a NFS filesystem has the danger to go OTW and thus defeat the whole purpose of what we're trying to achive right now. > 2) > The more I am looking to umountall(1M) the more I think that umountall(1M) > should get a new syscall and all the work apart from parsing the command > line options should be done in the kernel. > > Now, the kernel exports its state via a special file system in /etc/mnttab. > Information from /etc/mnttab is parsed as a text in umountall(1M) - parsing > is error prone - space can do lot of harm - see 6786256. After the > information is parsed > it is processed with low efficiency just to get a list of mount points > to be unmounted. > This list goes back to the kernel via umount(1). Current shell > implementation is slow and hard to maintain. this has been annoying me for a long time, we do have the in kernel mnttab and we do have the inkernel sharetab ! whats the story about this ancient shell script anyways these days ! I think we definitively should at least file a place holder RFE that we have a record of this would be a good idea to re-architect. --- frankB
[nfs-discuss] code review for 6778894 - NEW fix
Pavel Filipensky wrote: > A fresh fix is available at: > > http://cr.opensolaris.org/~pavelf/6778894-v2 (yes, the previous > and fragile v2 workspace is lost) > Two notes: 1) I am wondering if I should add this comment as a warning for future code changes which would go OTW and cause regression of "6675447 NFSv4 client hangs on shutdown if server is down beforehand." # we cannot us 'df -F nfs $mountp' to test if the given mountpoint has NFS on top of it # since df could trigger NFS over-the-wire request and cause regression of # "6675447 NFSv4 client hangs on shutdown if server is down beforehand." 2) The more I am looking to umountall(1M) the more I think that umountall(1M) should get a new syscall and all the work apart from parsing the command line options should be done in the kernel. Now, the kernel exports its state via a special file system in /etc/mnttab. Information from /etc/mnttab is parsed as a text in umountall(1M) - parsing is error prone - space can do lot of harm - see 6786256. After the information is parsed it is processed with low efficiency just to get a list of mount points to be unmounted. This list goes back to the kernel via umount(1). Current shell implementation is slow and hard to maintain. E.g. autofs does the unmounting in the kernel. There are two pending bugs which would profit from the new umounatll syscall: 6733798 manpage umountall(1M) - specify what is a local/remote filesystem + Interface stability 6786256 umountall(1M) handles incorrectly mounpoints with a space character in the path --Pavel
[nfs-discuss] code review for 6778894 - NEW fix
A fresh fix is available at:
http://cr.opensolaris.org/~pavelf/6778894-v2 (yes, the previous and
fragile v2 workspace is lost)
This fix is short and easy to read.
Performance of the fix is good, I have a setup with 1.000 and 5.000 NFS
mounts and 14 and 78 autofs mounts.
I was measuring the time of 'umountall -ln'
1)
nfs mounts: 1.000
autofs mounts: 14
time:
real0m8.071s
user0m7.233s
sys 0m0.720s
time without the fix:
real0m1.522s
user0m0.866s
sys 0m0.721s
2)
nfs mounts: 5.000
autofs mounts: 14
time:
real2m3.760s
user2m1.470s
sys 0m2.217s
time without the fix:
real0m5.312s
user0m3.385s
sys 0m2.086s
3)
nfs mounts: 1.000
autofs mounts: 78
time:
real0m8.612s
user0m7.577s
sys 0m0.972s
time without the fix:
real0m1.972s
user0m1.099s
sys 0m0.953s
4)
nfs mounts: 5.000
autofs mounts: 78
time:
real2m12.247s
user2m1.785s
sys 0m3.325s
time with the previous fix based on string_grep()
real2m2.689s
user2m0.188s
sys 0m2.468s
time without the fix:
real0m5.429s
user0m3.390s
sys 0m2.248s
Perfomance testing shows that the most important is the number of NFS
mounts,
number of autos mounts is less important.
The costly operation is the memory allocation for the nfs list at line 288:
288 +NFS_LIST="$NFS_LIST $mountp"
For comparison, umountall -rn (unmount only nfs) need also similar time
- again it is also doing lot of malloc in a loop on line 331
331 fslist="$fslist $mountp"
My fix:
real2m16.532s
user1m58.620s
sys 0m41.238s
Original:
real2m10.853s
user1m53.127s
sys 0m41.642s
--Pavel
> This is what you added:
>
> 239 +# Find if a given string is in a pipe separated list
> 240 +# Usage: string_grep string pattern
> 241 +# Example:
> 242 +# string_grep /var/tmp "/var/tmp|/net/server.domain|/home/user"
> 243 +string_grep() {
> 244 +eval "case $1 in '$2') return 0;; esac; return 1"
> 245 +}
>
> Try:
>
> string_grep() {
> eval "case \"$1\" in $2) return 0;; esac; return 1"
> }
>
>
The quotes arround the '$2' were only in my debug workspace and I have
put it incorrectly to cr.opensolaris.org.
Testing which was done without the quotes showed a problem if a
mountpoint was "/*" - it was matching
all mountpoints.
> The difference: a) cause $1 to be quoted in the eval'ed string, b) cause
> $2 not to be so quoted.
>
[nfs-discuss] code review for 6778894 - NEW fix
On Wed, Dec 17, 2008 at 06:31:05PM +0100, Pavel Filipensky wrote: > I have updated the comments, new webrev is here (it also contains the > latest umountall changeset from today): > > http://cr.opensolaris.org/~pavelf/6778894-v3 > > Can you give an explicit review of this workspace? You need to explicitly set NFS_LIST= the empty string, otherwise if NFS_LIST happens to be set in the environment when unmountall runs... I think I have a better way to deal with the whitespace issues too, including newlines. I'll send a reply on the other thread sometime after lunch.
[nfs-discuss] code review for 6778894 - NEW fix
On Wed, Dec 17, 2008 at 10:03:09AM -0600, Nicolas Williams wrote:
> To make it easier for you I wrote you the attached function to
> backslash-quote white-space in strings. That way you can change the
> main body of unmountall to:
The attachment didn't make the list, and though you have it, I forgot to
remove debug output. So here it is inlined.
bquote () {
orig=$1
set -- $1
bquoted=$1
shift
while [ $# -gt 0 ]
do
case "$orig" in
$bquoted\ ${1}*) bquoted="$bquoted\\ $1";;
# IF YOU CUT-N-PASTE do make SURE you fix spaces
# back to tabs in the next line!
$bquoted\ ${1}*) bquoted="$bquoted\\ $1";;
*) return 1;;
esac
shift
done
return 0
}
You can test it like so:
while read a b c
do
bquote "$a"
a=$bquoted
bquote "$b"
b=$bquoted
bquote "$c"
c=$bquoted
echo "$a $b $c"
done <
[nfs-discuss] code review for 6778894 - NEW fix
On Wed, Dec 17, 2008 at 03:13:40PM +0100, Pavel Filipensky wrote: > 1) > I am wondering if I should add this comment as a warning for future > code changes which would go OTW and cause regression of "6675447 > NFSv4 client hangs on shutdown if server is down beforehand." > > > # we cannot us 'df -F nfs $mountp' to test if the given mountpoint has > NFS on top of it > # since df could trigger NFS over-the-wire request and cause regression of > # "6675447 NFSv4 client hangs on shutdown if server is down beforehand." Sure. > 2) > The more I am looking to umountall(1M) the more I think that > umountall(1M) should get a new syscall and all the work apart from > parsing the command line options should be done in the kernel. > > Now, the kernel exports its state via a special file system in > /etc/mnttab. Information from /etc/mnttab is parsed as a text in > umountall(1M) - parsing is error prone - space can do lot of harm - > see 6786256. After the information is parsed it is processed with low > efficiency just to get a list of mount points to be unmounted. This > list goes back to the kernel via umount(1). Current shell > implementation is slow and hard to maintain. I wouldn't worry about this too much. If you use the shell built-in read function, double quote expansions of variables containing mount point paths, and the kernel backslash-quotes whitespace in mount point paths in /etc/mnttab then you're OK. > There are two pending bugs which would profit from the new umounatll > syscall: > > 6733798 manpage umountall(1M) - specify what is a local/remote > filesystem + Interface stability > 6786256 umountall(1M) handles incorrectly mounpoints with a space > character in the path umountall(1M) uses read, but not correctly: - first it reads a line at a time from mnttab to reverse the order of mnttab - then it echos the reversed list to a pipe to a shell function that parses each line The problem with that is that any backslash quoting in the original will be lost in the first read. The Korn shell has a -r argument to read that prevents this, but you can't use the Korn shell. You could move /bin/tac to /usr/sbin and then use tac(1) to do the reversing. THEN the read in domounts() will see any backslashes and work correctly. To make it easier for you I wrote you the attached function to backslash-quote white-space in strings. That way you can change the main body of unmountall to: 249 246 while read dev mountp fstype mode dummy if [ ! -x /usr/bin/tail ]; then exec < $MNTTAB REVERSED= while read dev mountp rest; do bquote mountp if [ -n "$REVERSED" ]; then REVERSED="$dev $bquoted $rest\n$REVERSED" else REVERSED="$dev $bquoted $rest" fi done error=`echo $REVERSED | doumounts` else error=`tail -r $MNTTAB | doumounts` fi Yes, I'm assuming that mnttab backslash-quotes whitespace. Nico --
