Re: [PATCH] Append both nofail and x-systemd.before to kdump mount target

[Kairui Song]

On Thu, May 14, 2020 at 4:43 PM piliu <piliu(a)redhat.com&gt; wrote:
>  On 05/13/2020 11:07 PM, Kairui Song wrote:
>  > Yes, I found x-systemd.before=kdump-capture.service is not enough to
>  > make sure the mount point is setup correctly before dracut-pre-pivot
>  > hooks, these hooks may clean up & kill service which may fail any
>  > mount opretions later.
>  >
>  > In the dracut boot progress graph:
>  >       (custom initrd services)
>  >                        |                       v
>  >                        |            initrd\-parse\-etc\&.service
>  >                        |                       |
>  >                        |                       v
>  >                        |            (sysroot\-usr\&.mount and
>  >                        |             various mounts marked
>  >                        |               with fstab option
>  >                        |                x\-initrd\&.mount)
>  >                        |                       |
>  >                        |                       v
>  >                        |                initrd\-fs\&.target
>  >                        \e______________________ |
>  >                                               \e|
>  >                                                v
>  >                                           initrd\&.target
>  >                                                |
>  >                                                v
>  >                                     dracut\-pre\-pivot\&.service
>  >                                                |
>  >                                                v
>  >                                      initrd\-cleanup\&.service
>  >                                           isolates to
>  >                                     initrd\-switch\-root\&.target
>  >
>  > dracut-pre-pivot hooks arqe after initrd.target, and custom initrd
>  > services are before it, and (various mounts marked with fstab) is also
>  > before it. So setting the mount to be finished before that seems a
>  > good idea.
>  >
>  > And a second though, maybe initrd-fs.target is a better choice? That
>  > should be the original dependency if "nofail" is not added.
>  Thanks for your explanation. It sounds better to depend on
>  initrd-fs.target. Would you send V3 for it? Or modify in your git repo.
>
>  For V3, you can add "Acked-by: Pingfan Liu <piliu(a)redhat.com&gt;&quot; directly
>
>  Thanks,
>  Pingfan
>
>  >
>  > On Wed, May 13, 2020 at 9:34 PM piliu <piliu(a)redhat.com&gt; wrote:
>  >>
>  >>
>  >>
>  >> On 05/13/2020 05:32 PM, Kairui Song wrote:
>  >>> On Mon, May 11, 2020 at 9:43 PM piliu <piliu(a)redhat.com&gt; wrote:
>  >>>> On 05/11/2020 02:42 PM, Kairui Song wrote:
>  >>>>> By this point, there is still an unresolved vfs kernel issue that
> blocks
>  >>>>> systemd from mounting the dump target properly from time to time.
> To
>  >>>>> prevent systemd from failing by mounting the dump target, we can
> add
>  >>>>> nofail option to the kdump mount point.
>  >>>>>
>  >>>>> But adding nofail will wipe out default dependency of the mount
> point,
>  >>>>> see commit 94a7b43, so systemd randomize the order of calling
> kdump.sh
>  >>>>> and mounting the dump target and lead to unexpected behavior.
>  >>>>> However we can use x-systemd.before to ensure the mount is done
>  >>>>> in right order.
>  >>>>>
>  >>>>> With both nofail and x-systemd.before=kdump-capture.service,
> systemd
>  >>>>> will try to mount the dump target before calling kdump, and even if
> the
>  >>>>> mount failed, kdump.sh still be called and try to mount again. See
>  >>>>> dump_fs function, which will try to mount if the target is not
> mounted.
>  >>>>> Kdump will only fail if both mount attemp fails.
>  >>>> I had thought about the asymmetry after the previous partial revert
>  >>>> patch, and its value. And this patch resolves it!
>  >>>>>
>  >>>>> Else if the kdump target mount failed or unstable, systemd will
> directly
>  >>>>> jump to kdump failure action, and kdump fails.
>  >>>>>
>  >>>>> This should improve the robustness in general with no other risk.
>  >>>>> ---
>  >>>>>  mkdumprd | 3 +++
>  >>>>>  1 file changed, 3 insertions(+)
>  >>>>>
>  >>>>> diff --git a/mkdumprd b/mkdumprd
>  >>>>> index cedf536..26e71ba 100644
>  >>>>> --- a/mkdumprd
>  >>>>> +++ b/mkdumprd
>  >>>>> @@ -83,6 +83,9 @@ to_mount() {
>  >>>>>      # drop nofail or nobootwait
>  >>>>>      _options=$(echo $_options | sed
> 's/\(^\|,\)nofail\($\|,\)/\1/g')
>  >>>>>      _options=$(echo $_options | sed
> 's/\(^\|,\)nobootwait\($\|,\)/\1/g')
>  >>>>> +    # use both nofail and x-systemd.before to ensure systemd will
> try best to
>  >>>>> +    # mount it before kdump starts, this is an attempt to improve
> robustness
>  >>>>> +   
> _options="$_options,nofail,x-systemd.before=kdump-capture.service"
>  >>>>>
>  >>>>>      _mntopts="$_target $_fstype $_options"
>  >>>>>      #for non-nfs _dev converting to use udev persistent name
>  >>>>>
>  >>>> Acked-by: Pingfan Liu <piliu(a)redhat.com&gt;
>  >>>>
>  >>>
>  >>> Hi Pingfan, thank you very much for the review, however I found there
>  >>> are still a timing issue with this patch, I've send V2, could you help
>  >>> have a look?
>  >> Sure, and could you pay some words about
>  >> "x-systemd.before=initrd.target" instead of
>  >> "x-systemd.before=kdump-capture.service"?  And what is the timing
> issue
>  >> you experienced?
>  >>
>  >> Thanks
>  >>>
>  >>
>  >
>  >
>  
OK, thanks for the review!

-- 
Best Regards,
Kairui Song