Re: [PATCH] kdump.conf: introduce new "nfs_mntopts" in "/etc/kdump.conf"

On 2016/05/11 at 14:08, Dave Young wrote: > On 05/11/16 at 11:16am, Xunlei Pang wrote: >> On 2016/05/11 at 10:50, Dave Young wrote: >>> Hi, Xunlei, >>> >>> On 05/05/16 at 04:49pm, Xunlei Pang wrote: >>>> There are some complaints about nfs kdump that users must mount >>>> nfs beforehand, which can also cause some overhead to NFS server. >>>> >>>> To solve it, we added a new "nfs_mntopts" in "/etc/kdump.conf" to >>>> allow users to specify the nfs options instead of mounting it. The >>>> general rule about "nfs_mntopts" is: >>>> 1) "nfs_mntopts" has the highest priority, if it is specified, we >>>> always use it as the final mount option even if it is invalid >>>> (kdump will fail to start). >>>> 2) When no nfs mounted beforehand: >>>> a) If "nfs_mntopts" is specifed, do a temporary mount using it >>>> as the mount options. >>>> b) If "nfs_mntopts" is not specifed, do a temporary mount using >>>> no options(default option). >>>> If the temporary mount fails, kdump restart fails. >>>> 3) When nfs was mounted beforehand, parsing mounted nfs as before, >>>> but with the following extra logic: >>>> a) If "nfs_mntopts" is specified, validate it by doing a temporary >>>> mount: If it is valid, make it as the options passed to dracut >>>> instead of the mounted nfs(overridding it); If it is invalid, >>>> kdump restart fails. >>>> b) If "nfs_mntopts" is not specified, parsing mounted nfs as before. >>>> 4) After kdump is finished(started or failed), umount the temporary nfs >>>> mount immediately if any. >>>> >>>> The patch introduced three new functions to do the implementation: >>>> nfs_mount_prepare(), nfs_mount_finish(), and nfs_mount_cleanup(). >>>> >>>> Signed-off-by: Xunlei Pang <xlpang(a)redhat.com&gt; >>>> --- >>>> kdump.conf | 4 ++++ >>>> kdumpctl | 4 ++-- >>>> mkdumprd | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- >>>> 3 files changed, 81 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/kdump.conf b/kdump.conf >>>> index 54b581d..f56556b 100644 >>>> --- a/kdump.conf >>>> +++ b/kdump.conf >>>> @@ -18,6 +18,9 @@ >>>> # nfs <nfs mount> - Will mount fs and copy /proc/vmcore to >>>> # <mnt>/var/crash/%HOST-%DATE/, supports DNS. >>>> # >>>> +# nfs_mntopts <mount options> - Will mount nfs using this options >>>> +# even if there's a mounted nfs with different options. >>>> +# >>> We also support $SAVE_PATH mounts without kdump.conf targets, it cause things >>> hard to resolve. What if one does not use nfs in /etc/kdump.conf? >>> >>> Another issue is we should not add config options for nfs only, if we add it >>> we should add a general one.. >>> >>> Before taking a look at the code, let's sort out the design problems first.. >> OK, thanks for the tips. >> >> I will try to consider adding a general one, but we validate the options specified >> except for NFS target to address the reported NFS issue, because unlike the other >> targest NFS is not mounted locally, it does impose some burden to the same NFS >> server if thousands of requests are made simultaneously. What do you think? > Rethink about it, the complains mainly come from nfs, we can add one more > field to nfs directive like below instead of adding a new option: > nfs <nfs mount> <nfs mount options> sounds good > > But wait, it still does not solve Ben's diskless issues, in his case it will > rebuild initrd on each boot even with your patch. We need do sanity check about > fs size etc so we need to mount the filesystem. > > For users who want to bypass the sanity checking, adding another kdump.conf option > sounds bad, a /etc/sysconfig/kdump option to switch might be better. How about adding two more fields to nfs directive? nfs <nfs mount> [nfs mount options] [validate_needed] Since the particularity of NFS, we can add some details illustrating why we need this way.