Bug#641155: pu: package mdadm/3.1.4-1+8efb9d1+squeeze1

[Michael Tokarev]

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