Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] nfsiostat/mountstats: Drop autofs entries before calling
 compare_iostats()
To:     Kenneth D'souza <kdsouza@redhat.com>, linux-nfs@vger.kernel.org
References: <20200831170639.3962-1-kdsouza@redhat.com>
From:   Steve Dickson <SteveD@RedHat.com>
Message-ID: <fefe923b-6d1b-b02d-ef87-3ae5f606ba0e@RedHat.com>
Date:   Tue, 8 Sep 2020 11:27:58 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200831170639.3962-1-kdsouza@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


On 8/31/20 1:06 PM, Kenneth D'souza wrote:
> nfsiostat/mountstats can fail with below KeyError when old stat and new stat
> data go out of sync.
> 
> $ mountstats iostat 1 3
> 
> Traceback (most recent call last):
>   File "/usr/sbin/mountstats", line 1092, in <module>
>     res = main()
>   File "/usr/sbin/mountstats", line 1081, in main
>     return args.func(args)
>   File "/usr/sbin/mountstats", line 965, in iostat_command
>     print_iostat_summary(old_mountstats, mountstats, devices, sample_time)
>   File "/usr/sbin/mountstats", line 920, in print_iostat_summary
>     diff_stats = stats.compare_iostats(old_stats)
>   File "/usr/sbin/mountstats", line 528, in compare_iostats
>     if old_stats.__nfs_data['age'] > self.__nfs_data['age']:
> KeyError: 'age'
> 
> Steps to Reproduce:
> 
> 1> Add autofs mounts in /etc/fstab controlled via systemd.
> 
> nfs-server:/test1 /mnt1 nfs noauto,x-systemd.idle-timeout=2,x-systemd.automount 0 0
> nfs-server:/test2 /mnt2 nfs noauto,x-systemd.idle-timeout=2,x-systemd.automount 0 0
> nfs-server:/test3 /mnt3 nfs noauto,x-systemd.idle-timeout=2,x-systemd.automount 0 0
> 
> 2> Trigger the mounts via below command:
> 
> $ while :; do date; ls -lR /mnt* 2>&1 >/dev/null; sleep 3; done
> 
> 3> On other terminal run nfsiostat or mountstats command:
> 
> $ mountstats iostat 1 3
> $ nfsiostat 1 3
> 
> Frequent mount and umount can cause autofs entries to be processed in compare_iostats.
> We need to filter the devices list and drop autofs entries to fix the issue.
> This way we pass only nfs mounts and not autofs entries.
> 
> Signed-off-by: Kenneth D'souza <kdsouza@redhat.com>
Committed...

steved.
> ---
>  tools/mountstats/mountstats.py | 9 +++++----
>  tools/nfs-iostat/nfs-iostat.py | 5 ++++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/mountstats/mountstats.py b/tools/mountstats/mountstats.py
> index 00adc96b..25e92a19 100755
> --- a/tools/mountstats/mountstats.py
> +++ b/tools/mountstats/mountstats.py
> @@ -953,10 +953,11 @@ def print_iostat_summary(old, new, devices, time):
>          if not old or device not in old:
>              stats.display_iostats(time)
>          else:
> -            old_stats = DeviceData()
> -            old_stats.parse_stats(old[device])
> -            diff_stats = stats.compare_iostats(old_stats)
> -            diff_stats.display_iostats(time)
> +            if ("fstype autofs" not in str(old[device])) and ("fstype autofs" not in str(new[device])):
> +                old_stats = DeviceData()
> +                old_stats.parse_stats(old[device])
> +                diff_stats = stats.compare_iostats(old_stats)
> +                diff_stats.display_iostats(time)
>  
>  def iostat_command(args):
>      """iostat-like command for NFS mount points
> diff --git a/tools/nfs-iostat/nfs-iostat.py b/tools/nfs-iostat/nfs-iostat.py
> index 4f5e8a66..1df74ba8 100755
> --- a/tools/nfs-iostat/nfs-iostat.py
> +++ b/tools/nfs-iostat/nfs-iostat.py
> @@ -470,10 +470,13 @@ def parse_stats_file(filename):
>  def print_iostat_summary(old, new, devices, time, options):
>      stats = {}
>      diff_stats = {}
> +    devicelist = []
>      if old:
>          # Trim device list to only include intersection of old and new data,
>          # this addresses umounts due to autofs mountpoints
> -        devicelist = [x for x in old if x in devices]
> +        for device in devices:
> +            if "fstype autofs" not in str(old[device]):
> +                devicelist.append(device)
>      else:
>          devicelist = devices
>  
>