Message-ID: <20230410065211.GA3638819@hu-pkondeti-hyd.qualcomm.com>
Date:   Mon, 10 Apr 2023 12:22:11 +0530
From:   Pavan Kondeti <quic_pkondeti@...cinc.com>
To:     Chen Yu <yu.c.chen@...el.com>
CC:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Pavan Kondeti <quic_pkondeti@...cinc.com>,
        Len Brown <len.brown@...el.com>, Ye Bin <yebin10@...wei.com>,
        <linux-pm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Yifan Li <yifan2.li@...el.com>
Subject: Re: [PATCH] PM: hibernate: Do not get block device exclusively in
 test_resume mode

On Sun, Apr 09, 2023 at 10:29:37PM +0800, Chen Yu wrote:
> On 2023-04-06 at 12:02:01 +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 6, 2023 at 4:49 AM Chen Yu <yu.c.chen@...el.com> wrote:
> > >
> > > Hi Rafael,
> > > On 2023-04-05 at 20:37:32 +0200, Rafael J. Wysocki wrote:
> > > > On Sat, Apr 1, 2023 at 10:59 AM Chen Yu <yu.c.chen@...el.com> wrote:
> > > > >
> > > > > The system refused to do a test_resume because it found that the
> > > > > swap device has already been taken by someone else. Specificly,
> > > > > the swsusp_check()->blkdev_get_by_dev(FMODE_EXCL) is supposed to
> > > > > do this check.
> > > > >
> > > > > Steps to reproduce:
> > > > >  dd if=/dev/zero of=/swapfile bs=$(cat /proc/meminfo |
> > > > >        awk '/MemTotal/ {print $2}') count=1024 conv=notrunc
> > > > >  mkswap /swapfile
> > > > >  swapon /swapfile
> > > > >  swap-offset /swapfile
> > > > >  echo 34816 > /sys/power/resume_offset
> > > > >  echo test_resume > /sys/power/disk
> > > > >  echo disk > /sys/power/state
> > > > >
> > > > >  PM: Using 3 thread(s) for compression
> > > > >  PM: Compressing and saving image data (293150 pages)...
> > > > >  PM: Image saving progress:   0%
> > > > >  PM: Image saving progress:  10%
> > > > >  ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
> > > > >  ata1.00: configured for UDMA/100
> > > > >  ata2: SATA link down (SStatus 0 SControl 300)
> > > > >  ata5: SATA link down (SStatus 0 SControl 300)
> > > > >  ata6: SATA link down (SStatus 0 SControl 300)
> > > > >  ata3: SATA link down (SStatus 0 SControl 300)
> > > > >  ata4: SATA link down (SStatus 0 SControl 300)
> > > > >  PM: Image saving progress:  20%
> > > > >  PM: Image saving progress:  30%
> > > > >  PM: Image saving progress:  40%
> > > > >  PM: Image saving progress:  50%
> > > > >  pcieport 0000:00:02.5: pciehp: Slot(0-5): No device found
> > > > >  PM: Image saving progress:  60%
> > > > >  PM: Image saving progress:  70%
> > > > >  PM: Image saving progress:  80%
> > > > >  PM: Image saving progress:  90%
> > > > >  PM: Image saving done
> > > > >  PM: hibernation: Wrote 1172600 kbytes in 2.70 seconds (434.29 MB/s)
> > > > >  PM: S|
> > > > >  PM: hibernation: Basic memory bitmaps freed
> > > > >  PM: Image not found (code -16)
> > > > >
> > > > > This is because when using the swapfile as the hibernation storage,
> > > > > the block device where the swapfile is located has already been mounted
> > > > > by the OS distribution(usually been mounted as the rootfs). This is not
> > > > > an issue for normal hibernation, because software_resume()->swsusp_check()
> > > > > happens before the block device(rootfs) mount. But it is a problem for the
> > > > > test_resume mode. Because when test_resume happens, the block device has
> > > > > been mounted already.
> > > > >
> > > > > Thus remove the FMODE_EXCL for test_resume mode. This would not be a
> > > > > problem because in test_resume stage, the processes have already been
> > > > > frozen, and the race condition described in
> > > > > Commit 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > > is unlikely to happen.
> > > > >
> > > > > Fixes: 39fbef4b0f77 ("PM: hibernate: Get block device exclusively in swsusp_check()")
> > > > > Reported-by: Yifan Li <yifan2.li@...el.com>
> > > > > Signed-off-by: Chen Yu <yu.c.chen@...el.com>
> > > > > ---
> > > > >  kernel/power/hibernate.c | 18 +++++++++++-------
> > > > >  kernel/power/power.h     |  2 +-
> > > > >  kernel/power/swap.c      | 10 +++++++---
> > > > >  3 files changed, 19 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> > > > > index 793c55a2becb..f50456e72f0a 100644
> > > > > --- a/kernel/power/hibernate.c
> > > > > +++ b/kernel/power/hibernate.c
> > > > > @@ -683,22 +683,26 @@ static void power_down(void)
> > > > >                 cpu_relax();
> > > > >  }
> > > > >
> > > > > -static int load_image_and_restore(void)
> > > > > +static int load_image_and_restore(bool safe)
> > > >
> > > > It is not very clear why the argument is called "safe".
> > > >
> > > > Either this needs to be explained in a comment, or I would just call
> > > > it "exclusive" and rework the checks accordingly.
> > > >
> > > OK, I can change it to "exclusive". Pavan proposed to add a global
> > > variable snapshot_testing to indicate that the system is in test_resume mode,
> > > and we can check this flag to decide whether to open the block device
> > > exclusively or not. Then we don't have to add parameter for load_image_and_restore()
> > > nor swsusp_check(). Could you please give advice whether this is applicable?
> > 
> > Well, in that case, why don't you simply check pm_test_level?
> >
> After rethink about this further, it seems that the global variable snapshot_testing
> can not present the race condition described in 39fbef4b0f77 in a corner case, if
> we do like this:
> 
>      1. echo test_resume > /sys/power/disk
>      2. mkfs.ext4 -O mmp  /dev/sda -b 1024
>      3. mount /dev/sda /home/test
>      4. echo "/dev/sda" > /sys/power/resume
>  
> We will still see the kernel crash, because in step4, the software_resume()
> will open swap device non-exclusively because step1 has enabled snapshot_testing.
> 
> That is to say, to avoid the race condition, we should let software_resume() open
> the swap device exclusively no matter what the hibernation mode is.
> 
> Maybe fall back to add "exclusive" flag for load_image_and_restore()
> and swsusp_check() is simpler.
> 
> Pavan, what do you think?
> 
Right, If we directly use (hibernation_mode == HIBERNATION_TEST_RESUME)
condition, it would be a problem. I was saying, snapshot_test which
is a local variable in hibernate() needs to be made global so that
block device open / close can use flags approriately. Onething I did not
like was passing flags to swsusp_close(). Thats the reason for me
to cache the flags while opening the block device and using it in
the swsusp_close().

Thanks,
Pavan

Powered by blists - more mailing lists