Messages in this thread | | | Date | Sun, 9 Apr 2023 22:29:37 +0800 | From | Chen Yu <> | Subject | Re: [PATCH] PM: hibernate: Do not get block device exclusively in test_resume mode |
| |
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@intel.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@intel.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@intel.com> > > > > Signed-off-by: Chen Yu <yu.c.chen@intel.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?
thanks, Chenyu
| |