[PATCH V2 0/3] kdumpctl: force rebuild when dump target is modified

[Pratyush Anand]

When dump target is re-formatted then its UUID changes. These patches add
support to recognize such changes and then force initramfs rebuild, if
dracut argument has an UUID based root location.                                                                                        
Changes since v1:
-- Local variable is defined in shell function  now
-- raw target does not take persistent names "by-uuid" now
-- dracut arguments grep is more robust
-- When no UUID then warn and return

Pratyush Anand (3):
  mkdumprd: do not lookup in by-uuid dirs for raw device's persistent
    name
  kdumpctl: force rebuild in case of dynamic system modification
  kdumpctl: force rebuild in case of dump target modification

kdumpctl | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 mkdumprd | 14 ++++++++++----
 2 files changed, 60 insertions(+), 4 deletions(-)

[PATCH V2 1/3] mkdumprd: do not lookup in by-uuid dirs for raw device's persistent name

[Pratyush Anand]

raw devices are not mounted and also does not need to contain any
filesystem. So they may have UUIDs(when formatted) and may not have UUIDs
when raw. Therefore, do not look for persistent names by-uuid for raw
devices.

Signed-off-by: Pratyush Anand 
Suggested-by: Dave Young 
---
 mkdumprd | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/mkdumprd b/mkdumprd
index 6e3d9757f3d8..ad40d2872f98 100644
--- a/mkdumprd
+++ b/mkdumprd
@@ -30,14 +30,20 @@ perror() {
 }

get_persistent_dev() {
-    local i _tmp _dev
+    local i _tmp _dev _lookup_dirs

_dev=$(udevadm info --query=name --name="$1" 2>/dev/null)
     [ -z "$_dev" ] && {
         perror_exit "Kernel dev name of $1 is not found."
     }

-    for i in /dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*; do
+    if [[ $2 = "raw" ]];then
+	_lookup_dirs="/dev/mapper/* /dev/disk/by-id/*"
+    else
+	_lookup_dirs="/dev/mapper/* /dev/disk/by-uuid/* /dev/disk/by-id/*"
+    fi
+
+    for i in $_lookup_dirs; do
         _tmp=$(udevadm info --query=name --name="$i" 2>/dev/null)
         if [ "$_tmp" = "$_dev" ]; then
             echo $i
@@ -138,7 +144,7 @@ to_mount() {
     _mntopts="$_target $_fstype $_options"
     #for non-nfs _dev converting to use udev persistent name
     if [ -b "$_source" ]; then
-        _pdev="$(get_persistent_dev $_source)"
+        _pdev="$(get_persistent_dev $_source $_fstype)"
         if [ $? -ne 0 ]; then
             return 1
         fi
@@ -565,7 +571,7 @@ do
         dd if=$config_val count=1 of=/dev/null > /dev/null 2>&1 || {
             perror_exit "Bad raw disk $config_val"
         }
-        _praw=$(get_persistent_dev $config_val)
+        _praw=$(get_persistent_dev $config_val "raw")
         if [ $? -ne 0 ]; then
             exit 1
         fi

[PATCH V2 2/3] kdumpctl: force rebuild in case of dynamic system modification

[Pratyush Anand]

There could be some dynamic system modification, which may affect kdump
kernel boot process. In such situation initramfs must be rebulit on the
basis of changes.
Since most of these checking methods will use information from
TARGET_INITRD, therefore check for its existence in common code.

Signed-off-by: Pratyush Anand 
---
 kdumpctl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index ef18a2d4f6ce..614a816e344c 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -327,6 +327,13 @@ setup_target_initrd()
    fi
 }

+is_system_modified()
+{
+	[[ -f $TARGET_INITRD ]] || return 1
+
+	return 0
+}
+
 check_rebuild()
 {
    local extra_modules modified_files=""
@@ -388,6 +395,11 @@ check_rebuild()
    	fi
    done

+	is_system_modified
+	if [ $? -ne 0 ]; then
+		force_rebuild="1"
+	fi
+
    #check if target initrd has fadump support
    if [ "$DEFAULT_DUMP_MODE" = "fadump" ] && [ -f "$TARGET_INITRD" ]; then
    	initramfs_has_fadump=`lsinitrd -m $TARGET_INITRD | grep ^kdumpbase$ | wc -l`

[PATCH V2 3/3] kdumpctl: force rebuild in case of dump target modification

[Pratyush Anand]

kdump also passes persistent device mapping as --mount or --device argument
of dracut. However this persistent id (UUID) is changed if dump target is
re-formated.
kdumpctl must have a mechanism to recognise this modification, so that its
service restart is able to rebuild initramfs.

Testing:
a) Attach an IDE device, lets say it is /dev/sdb1
b) Format it as ext4
    # mkfs.ext4 /dev/sdb1
    # blkid /dev/sdb1
    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
c) Mount it into /mnt and create a var/crash directory in it.
    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
d) Add following line in /etc/kdump.conf
    ext4 /dev/sdb1
e) Restart kdumpctl
    # kdumpctl restart
f) crash
    # echo c > /proc/sysrq-trigger
g) Here you will be able save vmcore with or without this patch.
h) repeat step (b), (c), (e) and (f)
i) Now you will be able to save vmcore only when you have this patch in
your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
after reformatting.

Signed-off-by: Pratyush Anand 
---
 kdumpctl | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/kdumpctl b/kdumpctl
index 614a816e344c..303626a276fe 100755
--- a/kdumpctl
+++ b/kdumpctl
@@ -327,10 +327,48 @@ setup_target_initrd()
    fi
 }

+is_dump_target_modified()
+{
+	local _target _dracut_args _uuid
+
+	_target=$(egrep "^ext[234]|^xfs|^btrfs" /etc/kdump.conf)
+
+	#if dump target does not exist then do not rebuild
+	[[ -n "$_target" ]] || return 0
+
+	_dracut_args=$(lsinitrd $TARGET_INITRD | grep "^Arguments:" | head -1)
+	if [[ -z $_dracut_args ]];then
+		echo "Warning: No dracut arguments found in initrd"
+		return 0
+	fi
+	#if --mount or --device is not by UUID, then also do not rebuild
+	echo $_dracut_args | grep "by-uuid" &> /dev/null
+	[[ $? -eq 0 ]] || return 0
+
+	_target=$(echo $_target | cut -d ' ' -f 2)
+	_uuid=$(blkid $_target | awk -F"UUID=" '{print $2}' | cut -d '"' -f 2)
+	if [[ -z $_uuid ]];then
+		echo "Warning: UUID is not present"
+		return 1
+	fi
+
+	echo $_dracut_args  | grep $_uuid &> /dev/null
+	#if dracut argument and _target have same uuid, then also do not rebuild
+	[[ $? -ne 0 ]] || return 0
+
+	return 1
+}
+
 is_system_modified()
 {
    [[ -f $TARGET_INITRD ]] || return 1

+	is_dump_target_modified
+	if [ $? -ne 0 ]; then
+	        echo "Detected change in dump target"
+	        return 1
+	fi
+
    return 0
 }

[PATCH V2 3/3] kdumpctl: force rebuild in case of dump target modification

[Dave Young]

Hi, Pratyush

Seem one comment inline about variable quotation, otherwise I'm fine.

On 04/11/16 at 03:54pm, Pratyush Anand wrote:
> kdump also passes persistent device mapping as --mount or --device argument
> of dracut. However this persistent id (UUID) is changed if dump target is
> re-formated.
> kdumpctl must have a mechanism to recognise this modification, so that its
> service restart is able to rebuild initramfs.
>
> Testing:
> a) Attach an IDE device, lets say it is /dev/sdb1
> b) Format it as ext4
>    # mkfs.ext4 /dev/sdb1
>    # blkid /dev/sdb1
>    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
> c) Mount it into /mnt and create a var/crash directory in it.
>    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
> d) Add following line in /etc/kdump.conf
>    ext4 /dev/sdb1
> e) Restart kdumpctl
>    # kdumpctl restart
> f) crash
>    # echo c > /proc/sysrq-trigger
> g) Here you will be able save vmcore with or without this patch.
> h) repeat step (b), (c), (e) and (f)
> i) Now you will be able to save vmcore only when you have this patch in
> your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
> after reformatting.
> Signed-off-by: Pratyush Anand
>
> kdumpctl | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/kdumpctl b/kdumpctl
> index 614a816e344c..303626a276fe 100755
> --- a/kdumpctl
> +++ b/kdumpctl
> @@ -327,10 +327,48 @@ setup_target_initrd()
>    fi
>  }
>
> +is_dump_target_modified()
> +{
> local _target _dracut_args _uuid
> _target=$(egrep "^ext[234]|^xfs|^btrfs" /etc/kdump.conf)
> #if dump target does not exist then do not rebuild
> [[ -n "$_target" ]] || return 0
> _dracut_args=$(lsinitrd $TARGET_INITRD | grep "^Arguments:" | head -1)
> if [[ -z $_dracut_args ]];then
> echo "Warning: No dracut arguments found in initrd"
> return 0
> fi
> #if --mount or --device is not by UUID, then also do not rebuild
> echo $_dracut_args | grep "by-uuid" &> /dev/null
> [[ $? -eq 0 ]] || return 0
> _target=$(echo $_target | cut -d ' ' -f 2)
> _uuid=$(blkid $_target | awk -F"UUID=" '{print $2}' | cut -d '"' -f 2)
> if [[ -z $_uuid ]];then

quote $_uuid
> echo "Warning: UUID is not present"
> return 1
> fi
> echo $_dracut_args  | grep $_uuid &> /dev/null
> #if dracut argument and _target have same uuid, then also do not rebuild
> [[ $? -ne 0 ]] || return 0
> return 1
>
> +}
>
> is_system_modified()
>  {
>    [[ -f $TARGET_INITRD ]] || return 1
> is_dump_target_modified
> if [ $? -ne 0 ]; then
>        echo "Detected change in dump target"
>        return 1
> fi
> return 0
>
> }
>
> -- 
> 2.5.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

Thanks
Dave

[PATCH V2 3/3] kdumpctl: force rebuild in case of dump target modification

[Pratyush Anand]

Hi Dave,

On 13/04/2016:04:57:58 PM, Dave Young wrote:
> Hi, Pratyush
>
> Seem one comment inline about variable quotation, otherwise I'm fine.
>
> On 04/11/16 at 03:54pm, Pratyush Anand wrote:
> > kdump also passes persistent device mapping as --mount or --device argument
> > of dracut. However this persistent id (UUID) is changed if dump target is
> > re-formated.
> > kdumpctl must have a mechanism to recognise this modification, so that its
> > service restart is able to rebuild initramfs.
> >
> > Testing:
> > a) Attach an IDE device, lets say it is /dev/sdb1
> > b) Format it as ext4
> >    # mkfs.ext4 /dev/sdb1
> >    # blkid /dev/sdb1
> >    /dev/sdb1: UUID="21c7baff-e35d-49c7-aa08-0fba4513f5bf" TYPE="ext4"
> > c) Mount it into /mnt and create a var/crash directory in it.
> >    # mount /dev/sdb1 /mnt;mkdir /mnt/var;mkdir /mnt/var/crash
> > d) Add following line in /etc/kdump.conf
> >    ext4 /dev/sdb1
> > e) Restart kdumpctl
> >    # kdumpctl restart
> > f) crash
> >    # echo c > /proc/sysrq-trigger
> > g) Here you will be able save vmcore with or without this patch.
> > h) repeat step (b), (c), (e) and (f)
> > i) Now you will be able to save vmcore only when you have this patch in
> > your kexec-tools. Your initramfs will be rebuilt when you repeat step (e)
> > after reformatting.
> > Signed-off-by: Pratyush Anand
> >
> > kdumpctl | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/kdumpctl b/kdumpctl
> > index 614a816e344c..303626a276fe 100755
> > --- a/kdumpctl
> > +++ b/kdumpctl
> > @@ -327,10 +327,48 @@ setup_target_initrd()
> >    fi
> >  }
> >
> > +is_dump_target_modified()
> > +{
> > local _target _dracut_args _uuid
> > _target=$(egrep "^ext[234]|^xfs|^btrfs" /etc/kdump.conf)
> > #if dump target does not exist then do not rebuild
> > [[ -n "$_target" ]] || return 0
> > _dracut_args=$(lsinitrd $TARGET_INITRD | grep "^Arguments:" | head -1)
> > if [[ -z $_dracut_args ]];then
> > echo "Warning: No dracut arguments found in initrd"
> > return 0
> > fi
> > #if --mount or --device is not by UUID, then also do not rebuild
> > echo $_dracut_args | grep "by-uuid" &> /dev/null
> > [[ $? -eq 0 ]] || return 0
> > _target=$(echo $_target | cut -d ' ' -f 2)
> > _uuid=$(blkid $_target | awk -F"UUID=" '{print $2}' | cut -d '"' -f 2)
> > if [[ -z $_uuid ]];then
>
> quote $_uuid

Thanks, will modify and send V3.

~Pratyush

[Dave Young]

Hi, Pratyush

The patchset looks good to me except the quotation in 3/3.

I think we can improve it more like below, but it can also be done later:
* The original checking files timestamp logic can be another function
  and we can call it in your  is_system_modified()

What do you think?

On 04/11/16 at 03:54pm, Pratyush Anand wrote:
> When dump target is re-formatted then its UUID changes. These patches add
> support to recognize such changes and then force initramfs rebuild, if
> dracut argument has an UUID based root location.                                                                                        
> Changes since v1:
> -- Local variable is defined in shell function  now
> -- raw target does not take persistent names "by-uuid" now
> -- dracut arguments grep is more robust
> -- When no UUID then warn and return
>
> Pratyush Anand (3):
>   mkdumprd: do not lookup in by-uuid dirs for raw device's persistent
>     name
>   kdumpctl: force rebuild in case of dynamic system modification
>   kdumpctl: force rebuild in case of dump target modification
>
> kdumpctl | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  mkdumprd | 14 ++++++++++----
>  2 files changed, 60 insertions(+), 4 deletions(-)
>
> -- 
> 2.5.0
> _______________________________________________
> kexec mailing list
> [e-mail]
> http://lists.fedoraproject.org/admin/lists/[e-mail]

Thanks
Dave

[Pratyush Anand]

Hi Dave,

On 13/04/2016:05:04:23 PM, Dave Young wrote:
> Hi, Pratyush
>
> The patchset looks good to me except the quotation in 3/3.
>
> I think we can improve it more like below, but it can also be done later:
> The original checking files timestamp logic can be another function
> and we can call it in your  is_system_modified()
>
> What do you think?

In the function is_system_modified(), we track modification in hardware
resources. So, probably it would be better to have another function
is_files_modified() where all those logic can be moved.

However, I should do another modification in V3. Currently I am setting
force_rebuild variable if system is modified. Meaning of force_rebuild is
something different, so probably it would be better to keep another variable like
system_modified for this purpose.

~Pratyush

[Dave Young]

On 04/13/16 at 02:47pm, Pratyush Anand wrote:
> Hi Dave,
>
> On 13/04/2016:05:04:23 PM, Dave Young wrote:
> > Hi, Pratyush
> >
> > The patchset looks good to me except the quotation in 3/3.
> >
> > I think we can improve it more like below, but it can also be done later:
> > The original checking files timestamp logic can be another function
> > and we can call it in your  is_system_modified()
> >
> > What do you think?
>
> In the function is_system_modified(), we track modification in hardware
> resources. So, probably it would be better to have another function
> is_files_modified() where all those logic can be moved.

They sounds somehow identical, files should belong to system?
> However, I should do another modification in V3. Currently I am setting
> force_rebuild variable if system is modified. Meaning of force_rebuild is
> something different, so probably it would be better to keep another variable like
> system_modified for this purpose.

Yes, it just works, but force_rebuild should mean about only *force* rebuild
for things like below: 
1) one specify force_rebuild  in /etc/kdump.conf
2) for cases we are not sure, such as one add extra_modules in /etc/kdump.conf
possible reason is we do not know the dependency of extra_modules so that we
are not sure we should check what modules about the timestamps thus just simply
force-rebuild the inird

But for things we can determine, another variable should be even better, but I
am not sure if the code will be complexer.

Thanks
Dave