On 14.01.2012 19:50, Adam D. Barratt wrote: > On Tue, 2012-01-10 at 12:33 +0400, Michael Tokarev wrote: >> So, after quite some time, I'm attempting another upload of mdadm to >> stable-pu. All the changes were sitting in testing for half a year, >> and were backported into stable/squeeze version. >> >> As before, the proposed changes are only in debian-specific areas of >> the package, fixing several bugs in supporting/maintenance scripts. >> Complete debdiff is attached. > > Thanks for this. A few (hopefully) quick queries. > > + * Fix checkarray script so that it does not die after scheduling the first > + device when there is no scheduling class specified; thanks to Mario > + 'BitKoenig' Holbe (closes: #611627). > > The fix for this includes this change: > > - ionice -p "$resync_pid" $arg > + ionice -p "$resync_pid" $arg || : > > I assume this is because $arg is unset and the ionice invocation thus > fails? Wouldn't it better to check that the arguments are sane before > attempting to run ionice instead?
No, it is not due to empty $arg. For the full details see the discussion in the bugreport mentioned: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=611627#17 It wasn't my bugfix, but now when I look at it, the whole logic in the inner loop appears to be questionable, or racy: echo $action > $SYNC_ACTION_CTL [ $quiet -lt 1 ] && echo "$PROGNAME: I: check queued for array $array." >&2 ... resync_pid= wait=5 while [ $wait -gt 0 ]; do wait=$((wait - 1)) resync_pid=$(ps -ef | awk -v dev=$array 'BEGIN { pattern = "^\\[" dev "_resync]$" } $8 ~ pattern { print $2 }') if [ -n "$resync_pid" ]; then echo "$PROGNAME: I: selecting $ionice I/O scheduling class for resync of $array." >&2 ionice -p "$resync_pid" $arg break fi sleep 1 done What it does here is triggers resync action and waits for the kernel resync thread to appear, since it might appear later. It waits for the kernel thread for up to 5 secs. Once it is found, checkarray tries to ionice this thread. The change you're talking about is to fix one half of the race condition: if the thread exits between the ps call but before ionice, so ionice reports error and - due to set -e in effect - whole thing terminates. What the script does not take into account is that the thread may start much later than 5 seconds, due to other arrays being checked/resynced at the same time for example -- and in this case no scheduling policy will be set for the thread in question. But this is still a minor race, in my opinion anyway. I dont want to change too much in there for squeeze at this stage. The change you're talking about is harmless. > + * Schedule start/stop of mdadm-raid before/after filesystems are > + checked&mounted/unmounted; thanks to Mario 'BitKoenig' Holbe > + (closes: #611632). > [...] > + * Make mdadm-raid init script depend on hostname; thanks to Mario > + 'BitKoenig' Holbe (closes: #610421). > > Have these changes to the LSB headers been tested on squeeze systems? I tested these briefly, no extensive tests were done. That's probably my mistake, but the problem is that it's difficult to come with some real testcases where this may go wrong. But lemme recheck it again, I'll send a follow-up email. > + * Schedule start/stop of mdadm-raid before/after filesystems are > + checked&mounted/unmounted; thanks to Mario 'BitKoenig' Holbe > + (closes: #611632). > > This change appears twice in the changelog. Is this purely an editorial > issue or is there another change which isn't mentioned in the changelog? I haven't noticed. This is purely due to merge error of two series of changes for debian/changelog. I'll fix this, thank you for spotting it. Thank you! /mjt -- To UNSUBSCRIBE, email to debian-release-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org Archive: http://lists.debian.org/4f11ac16.4020...@msgid.tls.msk.ru
