LinuxQuestions.org - [SOLVED] Improving a bash script

- Linux - Newbie (https://www.linuxquestions.org/questions/linux-newbie-8/)

- - Improving a bash script (https://www.linuxquestions.org/questions/linux-newbie-8/improving-a-bash-script-4175501191/)

Improving a bash script

Hi to all,
I started writing bash scripts recently, and my first serious attempt is this one.

Code:

 

#!/bin/bash



for disk in $(ls -la /dev/disk/by-uuid/ |grep -o "sd.*1");



do

        LoadCycle=$(smartctl -A /dev/$disk |grep Load_Cycle |awk '{print $10}')

        PowerOnHours=$(smartctl -A /dev/$disk |grep Power_On_Hours |awk '{print $10}')



        myvar=$(echo "scale=1; (60 * $PowerOnHours) / $LoadCycle" | bc)

        

        #echo $(awk '{print strtonum($myvar)}')

        echo $disk $myvar

done

Output

Code:

sdg1 1527.2

sdk1 24.4

sda1 2048.5

sdh1 3.5

sde1 7413.2

(standard_in) 2: syntax error

sdc1

(standard_in) 2: syntax error

sdb1

sdi1 242.7

sdj1 4645.4

sdd1 480.0

sdf1 1072.2

Background and Goal:
I have assembled a server from old hardware just for samba with 12 HD. The last 2 years, 4 of these disks fell apart (WD Greens) due to the well known problem of aggressive head parking.
So i replaced them, tweaked the firmware (wdidle), and i now want to observe how often the heads are parking.
I wrote the above script. The output is in minutes. For example sdk1 24.4 says that sdk1 parks the head every 24.4 mins (on average).

My questions are two.
1. sdb1 and sdc1 are old ATA drives, so they do not report Load_Cycle through smart. So i get (standard_in) 2: syntax error. I tried to use awk strtonum to turn the error string to zero value but i couldnt manage it. Do you have any suggestions?

2. The script runs fine but it needs 4-5 secs to run. Do you have any suggestions to improve its speed? At a later point I want to feed the numbers to munin, in order to have a graphical display of the values.

Thank you in advance for your help.

Welcome to LQ niaban, and I think you've done a very good job at writing a script.

I'm sure there are more sophisticated ways to do this. I say try to run smartctl on disks you know work and on disks you know don't work and check the return to the shell:

Code:

smartctl -i /dev/sdi1

echo $?

smartctl -i /dev/sdc1

echo $?

Presuming that works; if you see a different result for the attempt on sdc1 then you can use the variable $? in your script as a test variable.

Code:

#!/bin/bash



for disk in $(ls -la /dev/disk/by-uuid/ |grep -o "sd.*1");



do

        smartctl -i /dev/$disk

        if [ $? == 0 ]; then

            LoadCycle=$(smartctl -A /dev/$disk |grep Load_Cycle |awk '{print $10}')

            PowerOnHours=$(smartctl -A /dev/$disk |grep Power_On_Hours |awk '{print $10}')



            myvar=$(echo "scale=1; (60 * $PowerOnHours) / $LoadCycle" | bc)

        

            #echo $(awk '{print strtonum($myvar)}')

            echo $disk $myvar

        fi

done

You can also just assign the output of the smartctl -i command to a variable and test that variable; that is what the shell does and the variable is $?.

I agree with the above, except that if you only care about success or failure, then let 'if' do the work for you.

Do not use the output of ls, as it can be unpredictable. Why not just glob straight from /dev?

The use of grep and awk is not required as awk can already perform the matching task.

Code:

#!/bin/bash



for disk in /dev/sd*1

do

        if smartctl -i "$disk"; then

            LoadCycle=$(smartctl -A "$disk" |awk '/Load_Cycle/{print $10}')

            PowerOnHours=$(smartctl -A "$disk" | awk '/Power_On_Hours/{print $10}')



            myvar=$(echo "scale=1; (60 * $PowerOnHours) / $LoadCycle" | bc)

        

            #echo $(awk '{print strtonum($myvar)}')

            echo $disk $myvar

        fi

done

Now I am not familiar with smartctl, so the if may also need to be redirected to /dev/null if you do not wish to see any output.

Edit: on a short after thought, as you run the awk on the exact same data, why not let it do the calculation as it can already handle floating point:

Code:

myvar=$(smartctl -A "$disk" |awk '/Load_Cycle/{lc = $10}/Power_On_Hours/{ph = $10}END{print (60 * ph) / lc}')

Thank you both very much.
As, all disks are smart enabled, smartctl -i /dev/sdXX gives a result for all of them.
So, i have to stick with smartctl -A /dev/$disk | grep Load_Cycle

Indeed, if i let awk handle the calculation (as grail suggested), script goes from 5" to 2.5".
That was great!!!!

On the other hand i still had an error message for division by zero.
I followed your suggestions for the if statement and execution time went from 2.5" to 4"
At least i dont have the error.

Final code

Code:

 

for disk in $(ls /dev/sd*1);

do

  smartctl -A $disk | grep Load_Cycle >/dev/null

  if [ $? == 0 ]; then

    myvar=$(smartctl -A $disk |awk '/Load_Cycle/{lc = $10}/Power_On_Hours/{ph = $10}END{print (60 * ph) / lc}')

    echo $disk $myvar

  fi

done

I think you missed the part about do not use ls. Also in the case where you have used it, it is completely pointless as the glob will already expand to all the devices.

As for the divide by zero, remove the if all together and have awk deliver a zero when lc is zero

Code:

for disk in /dev/sd*1

do

  echo "$disk $(smartctl -A $disk |awk '/Load_Cycle/{lc = $10}/Power_On_Hours/{ph = $10}END{if(lc)print (60 * ph) / lc; else print 0}')"

done

thank you both very much