[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 --
[nfs-discuss] code review for 6778894
Here is a draft which works fine for normal mount points:
http://cr.opensolaris.org/~pavelf/6778894-v2/
It uses sh(1) pattern matching features:
sh(1)
case word in [ pattern [ | pattern ] ) list ;; ] ... esac
A case command executes the list associated with the
first pattern that matches word. The form of the pat-
terns is the same as that used for file-name generation
(see File Name Generation section), except that a slash,
a leading dot, or a dot immediately following a slash
need not be matched explicitly.
=
This solution is vulnerable to e.g. * expansion:
$ mkdir /\*
$ mount server:/export /\*
$ umountall -l
Nothing will be unmounted since every mountpoint will match against '/*'
and will be considered as a NFS mountpoint.
I do not expect that sysadmins use often '*' in names for mountpoints,
but such solution is not acceptable.
Any suggestions how to implement a robust grep in a shell?
--Pavel
Nicolas Williams wrote:
> On Tue, Dec 16, 2008 at 09:31:14PM +0100, Pavel Filipensky wrote:
>
>> If you provide a path to df it will use realpath() -> resolvepath(2) :
>>
>> $ truss df -n /WWW
>> resolvepath("/WWW", "/WWW", 1024) = 4
>>
>
> Ah!
>
>
>> But the point was to specify the path (mountpoint) to df, without it we
>> do not get any advantage from using df.
>> So we must parse the /etc/mnttab anyway.
>>
>
> OK. Glad I asked!
>
> Nico
>
[nfs-discuss] code review for 6778894
If you provide a path to df it will use realpath() -> resolvepath(2) :
$ truss df -n /WWW
resolvepath("/WWW", "/WWW", 1024) = 4
If we avoid the path, resolvepath(2) is not used:
# df -F nfs -n
/home/pf199842 : nfs
/WWW : nfs
Fast dtrace check shows that it will not reach nfs kernel module,
so it will not go OTW:
# dtrace -Fn':nfs::/pid==$target/{}' -c 'df -F nfs -n'
dtrace: description ':nfs::' matched 2393 probes
/home/pf199842 : nfs
/WWW : nfs
dtrace: pid 29299 has exited
But the point was to specify the path (mountpoint) to df, without it we
do not get any advantage from using df.
So we must parse the /etc/mnttab anyway.
--Pavel
Frank Batschulat (Home) wrote:
> On Tue, 16 Dec 2008 20:56:36 +0100, Pavel Filipensky sun.com> wrote:
>
>
>> Unfortunately df -n calls resolvepath(2) and this will go OTW.
>> I have only tested that the fix avoids unmounting of NFS when
>> -l is used, but I has not realized that it will hang if server is down.
>>
>
> Pavel, I fail to see a way that df.c calls resolvepath(2) on anything that
> is slightly NFS releated (sure it'll use it on startup for the
> shared libraries...)
>
> ---
> frankB
>
>
[nfs-discuss] code review for 6778894
On Tue, 16 Dec 2008 20:56:36 +0100, Pavel Filipensky wrote: > Unfortunately df -n calls resolvepath(2) and this will go OTW. > I have only tested that the fix avoids unmounting of NFS when > -l is used, but I has not realized that it will hang if server is down. Pavel, I fail to see a way that df.c calls resolvepath(2) on anything that is slightly NFS releated (sure it'll use it on startup for the shared libraries...) --- frankB
[nfs-discuss] code review for 6778894
Unfortunately df -n calls resolvepath(2) and this will go OTW. I have only tested that the fix avoids unmounting of NFS when -l is used, but I has not realized that it will hang if server is down. I apologize for the terrible mistake. I plan a new fix as follows: if (LFLAG && fstype==autofs && (in /etc/mnttab exists fs with same mountpoint and nfs type) SKIP so need some more shell scripting. Would be nice to have umountall in C (or even a new syscall) Thank for discovering that. --Pavel > Does it? truss doesn't show it doing that when the -n option is used, > it only fstat64()s stdout. > > If Pavel has tested this fix correctly then there may not be a hang at > all. > > >> probably better to parse the 'mount -p' output instead. >> > > Now that I've trussed df -n I am certain that it won't hang. > > Of course, it's important to make sure that df -n will never be modified > in such a way that it could hang, but I think such a change is extremely > unlikely. A block comment in df wouldn't hurt though. > > Nico >
[nfs-discuss] code review for 6778894
On Tue, 16 Dec 2008 18:48:13 +0100, Nicolas Williams wrote:
> On Tue, Dec 16, 2008 at 06:42:01PM +0100, Frank Batschulat (Home) wrote:
>> On Tue, 16 Dec 2008 18:28:02 +0100, Nicolas Williams > sun.com> wrote:
>>
>> > On Tue, Dec 16, 2008 at 02:22:20PM +0100, Pavel Filipensky wrote:
>> >> this is a fix for "6778894 umountall -l still not really local yet".
>> >> webrev: http://cr.opensolaris.org/~pavelf/6778894/
>> >>
>> >> Background: My recent fix for "6675447 NFSv4 client hangs
>> >> on shutdown if server is down beforehand" is built on "umountall -l",
>> >> but as noted above umountall -l still not really local yet.
>> >> Unless umountall -l is fixed, the NFSv4 client can still hang
>> >> on shutdown if server is down beforehand.
>> >
>> > So df -n doesn't hang when NFS servers are non-responsive?
>>
>> outch! Thanks !
>>
>> Yes, of course, df uses statvfs() and statvfs() will go OTW for NFS.
>
> Does it? truss doesn't show it doing that when the -n option is used,
> it only fstat64()s stdout.
yepp, df gets this information of the mnttab entry, on start
it read in the entire mnttab, the entries itself provide anything
it needs to know for 'df -n'
69 struct extmnttab {
70 char*mnt_special;
71 char*mnt_mountp;
72 char*mnt_fstype;
73 char*mnt_mntopts;
74 char*mnt_time;
75 uint_t mnt_major;
76 uint_t mnt_minor;
77 };
connects this internally to:
157 struct mtab_entry {
158 bool_intmte_dev_is_valid;
159 dev_t mte_dev;
160 bool_intmte_ignore; /* the "ignore" option was set
*/
161 struct extmnttab*mte_mount;
162 };
from this it builds the df_request:
165 struct df_request {
166 bool_intdfr_valid;
167 char*dfr_cmd_arg; /* what the user
specified */
168 struct mtab_entry *dfr_mte;
169 char*dfr_fstype;
170 int dfr_index; /* to make qsort stable
*/
171 };
and the output callback for '-n' finally does:
1636 n_output(struct df_request *dfrp, struct statvfs64 *fsp)
1637 {
1638 (void) printf("%-*s: %-*s\n",
1639 MOUNT_POINT_WIDTH, DFR_MOUNT_POINT(dfrp),
1640 FSTYPE_WIDTH, dfrp->dfr_fstype);
1641 }
173 #define DFR_MOUNT_POINT(dfrp) (dfrp)->dfr_mte->mte_mount->mnt_mountp
we are safe...puh
---
frankB
[nfs-discuss] code review for 6778894
On Tue, 16 Dec 2008 18:28:02 +0100, Nicolas Williams wrote: > On Tue, Dec 16, 2008 at 02:22:20PM +0100, Pavel Filipensky wrote: >> this is a fix for "6778894 umountall -l still not really local yet". >> webrev: http://cr.opensolaris.org/~pavelf/6778894/ >> >> Background: My recent fix for "6675447 NFSv4 client hangs >> on shutdown if server is down beforehand" is built on "umountall -l", >> but as noted above umountall -l still not really local yet. >> Unless umountall -l is fixed, the NFSv4 client can still hang >> on shutdown if server is down beforehand. > > So df -n doesn't hang when NFS servers are non-responsive? outch! Thanks ! Yes, of course, df uses statvfs() and statvfs() will go OTW for NFS. probably better to parse the 'mount -p' output instead. --- frankB
[nfs-discuss] code review for 6778894
On Tue, Dec 16, 2008 at 11:26:49PM +0100, Pavel Filipensky wrote:
> Here is a draft which works fine for normal mount points:
>
> http://cr.opensolaris.org/~pavelf/6778894-v2/
...
> Any suggestions how to implement a robust grep in a shell?
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 difference: a) cause $1 to be quoted in the eval'ed string, b) cause
$2 not to be so quoted.
% string_grep foo\* f\* && echo hi
hi
% string_grep foo\* foobar\* && echo hi
%
Nico
--
[nfs-discuss] code review for 6778894
Hi Marcel, > I think the wording here is confusing a bit: > > 297 +# df -n produces following output > 298 +# df -n /WWW > 299 +# /WWW : nfs > > I suggest to change it to something like: > > # "df -n /WWW" produces following output > # /WWW : nfs > Changed. > > Also, are you sure about the "df -n" output stability? > I will check it. Thanks Pavel
[nfs-discuss] code review for 6778894
Hi Pavel, On Tue, Dec 16, 2008 at 02:22:20PM +0100, Pavel Filipensky wrote: > Hi, > > this is a fix for "6778894 umountall -l still not really local yet". > webrev: http://cr.opensolaris.org/~pavelf/6778894/ I think the wording here is confusing a bit: 297 +# df -n produces following output 298 +# df -n /WWW 299 +# /WWW : nfs I suggest to change it to something like: # "df -n /WWW" produces following output # /WWW : nfs Also, are you sure about the "df -n" output stability? > > Background: My recent fix for "6675447 NFSv4 client hangs > on shutdown if server is down beforehand" is built on "umountall -l", > but as noted above umountall -l still not really local yet. > Unless umountall -l is fixed, the NFSv4 client can still hang > on shutdown if server is down beforehand. Regards. -- Marcel Telka Solaris RPE
[nfs-discuss] code review for 6778894
Thanks Frank. I have added: 71 # 72 # If -l is specified, /usr/sbin/df is used to check the type of the file 73 # system. This extra test is skipped if /usr/sbin/df is not available. 74 # This is acceptable. and updated the webrev: http://cr.opensolaris.org/~pavelf/6778894/ --Pavel Frank Batschulat (Home) wrote: > On Tue, 16 Dec 2008 14:22:20 +0100, Pavel Filipensky sun.com> wrote: > > >> this is a fix for "6778894 umountall -l still not really local yet". >> webrev: http://cr.opensolaris.org/~pavelf/6778894/ >> >> Background: My recent fix for "6675447 NFSv4 client hangs >> on shutdown if server is down beforehand" is built on "umountall -l", >> but as noted above umountall -l still not really local yet. >> Unless umountall -l is fixed, the NFSv4 client can still hang >> on shutdown if server is down beforehand. >> > > minor nit, please extend the intro comment: > > 59 # The following /usr based commands may be used by this script > (depending on > 60 # command line options). We will set our PATH to find them, but where > they > 61 # are not present (eg, if /usr is not mounted) we will catch references > to > 62 # them via shell functions conditionally defined after option processing > 63 # (don't use any of these commands before then). > 64 # > 65 # Command Command line option and use > 66 # /usr/bin/sleep-k, to sleep after an fuser -c -k on the > mountpoint > 67 # /usr/sbin/fuser -k, to kill processes keeping a mount point busy > > to mention the use of /usr/sbin/df too. > > other then that, ship it! > > --- > frankB > > ___ > nfs-discuss mailing list > nfs-discuss at opensolaris.org >
[nfs-discuss] code review for 6778894
On Tue, 16 Dec 2008 14:22:20 +0100, Pavel Filipensky wrote: > this is a fix for "6778894 umountall -l still not really local yet". > webrev: http://cr.opensolaris.org/~pavelf/6778894/ > > Background: My recent fix for "6675447 NFSv4 client hangs > on shutdown if server is down beforehand" is built on "umountall -l", > but as noted above umountall -l still not really local yet. > Unless umountall -l is fixed, the NFSv4 client can still hang > on shutdown if server is down beforehand. minor nit, please extend the intro comment: 59 # The following /usr based commands may be used by this script (depending on 60 # command line options). We will set our PATH to find them, but where they 61 # are not present (eg, if /usr is not mounted) we will catch references to 62 # them via shell functions conditionally defined after option processing 63 # (don't use any of these commands before then). 64 # 65 # Command Command line option and use 66 # /usr/bin/sleep-k, to sleep after an fuser -c -k on the mountpoint 67 # /usr/sbin/fuser -k, to kill processes keeping a mount point busy to mention the use of /usr/sbin/df too. other then that, ship it! --- frankB
[nfs-discuss] code review for 6778894
On Tue, Dec 16, 2008 at 09:31:14PM +0100, Pavel Filipensky wrote:
> If you provide a path to df it will use realpath() -> resolvepath(2) :
>
> $ truss df -n /WWW
> resolvepath("/WWW", "/WWW", 1024) = 4
Ah!
> But the point was to specify the path (mountpoint) to df, without it we
> do not get any advantage from using df.
> So we must parse the /etc/mnttab anyway.
OK. Glad I asked!
Nico
--
[nfs-discuss] code review for 6778894
Hi, this is a fix for "6778894 umountall -l still not really local yet". webrev: http://cr.opensolaris.org/~pavelf/6778894/ Background: My recent fix for "6675447 NFSv4 client hangs on shutdown if server is down beforehand" is built on "umountall -l", but as noted above umountall -l still not really local yet. Unless umountall -l is fixed, the NFSv4 client can still hang on shutdown if server is down beforehand. Thanks, Pavel
[nfs-discuss] code review for 6778894
On Tue, Dec 16, 2008 at 08:56:36PM +0100, Pavel Filipensky wrote: > Unfortunately df -n calls resolvepath(2) and this will go OTW. Hmmm, truss doesn't show that. It does call resolvepath(2), but only as a result of loading shared objects. Nico --
[nfs-discuss] code review for 6778894
On Tue, Dec 16, 2008 at 06:42:01PM +0100, Frank Batschulat (Home) wrote: > On Tue, 16 Dec 2008 18:28:02 +0100, Nicolas Williams sun.com> wrote: > > > On Tue, Dec 16, 2008 at 02:22:20PM +0100, Pavel Filipensky wrote: > >> this is a fix for "6778894 umountall -l still not really local yet". > >> webrev: http://cr.opensolaris.org/~pavelf/6778894/ > >> > >> Background: My recent fix for "6675447 NFSv4 client hangs > >> on shutdown if server is down beforehand" is built on "umountall -l", > >> but as noted above umountall -l still not really local yet. > >> Unless umountall -l is fixed, the NFSv4 client can still hang > >> on shutdown if server is down beforehand. > > > > So df -n doesn't hang when NFS servers are non-responsive? > > outch! Thanks ! > > Yes, of course, df uses statvfs() and statvfs() will go OTW for NFS. Does it? truss doesn't show it doing that when the -n option is used, it only fstat64()s stdout. If Pavel has tested this fix correctly then there may not be a hang at all. > probably better to parse the 'mount -p' output instead. Now that I've trussed df -n I am certain that it won't hang. Of course, it's important to make sure that df -n will never be modified in such a way that it could hang, but I think such a change is extremely unlikely. A block comment in df wouldn't hurt though. Nico --
[nfs-discuss] code review for 6778894
On Tue, Dec 16, 2008 at 02:22:20PM +0100, Pavel Filipensky wrote: > this is a fix for "6778894 umountall -l still not really local yet". > webrev: http://cr.opensolaris.org/~pavelf/6778894/ > > Background: My recent fix for "6675447 NFSv4 client hangs > on shutdown if server is down beforehand" is built on "umountall -l", > but as noted above umountall -l still not really local yet. > Unless umountall -l is fixed, the NFSv4 client can still hang > on shutdown if server is down beforehand. So df -n doesn't hang when NFS servers are non-responsive? Nico --
[nfs-discuss] code review for 6778894
Pavel Filipensky wrote: > Thanks Frank. I have added: > > 71 # > 72 # If -l is specified, /usr/sbin/df is used to check the type of the file > 73 # system. This extra test is skipped if /usr/sbin/df is not available. > 74 # This is acceptable. > You don't need line 74.
