Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 6 Apr 2023 10:49:22 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     "Rafael J. Wysocki" <rafael@kernel.org>
CC:     Len Brown <len.brown@intel.com>, Ye Bin <yebin10@huawei.com>,
        <linux-pm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Yifan Li <yifan2.li@intel.com>,
        Pavan Kondeti <quic_pkondeti@quicinc.com>
Subject: Re: [PATCH] PM: hibernate: Do not get block device exclusively in
 test_resume mode
Message-ID: <ZC4zMv1SCiUMbcqs@chenyu5-mobl1>
References: <20230401165540.322665-1-yu.c.chen@intel.com>
 <CAJZ5v0j704JoCPTrDO1jTHwP1+8NVdLk7hym5036uFcfo7YHXQ@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAJZ5v0j704JoCPTrDO1jTHwP1+8NVdLk7hym5036uFcfo7YHXQ@mail.gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dzFZanNHT2pNSmxrRGcvS21OZEpwU2tNWjNacFNxcExkTnhrTUxGbGJ4K2lz?=
 =?utf-8?B?Zlo4cDFqNVMyK25PRzF4TmFpQTIrdW5GTjBTaFNJdGFwd0swWnhGc3ZFcTNy?=
 =?utf-8?B?NGNDeW9TU1JCMmxOKzhwdDNlUy85OVJqSFBYNll4TkxFTUo1T3VwbTdzQ1c3?=
 =?utf-8?B?R1ZOT1h6WGRTd0xMR0NVaU5EdEZzQWY0UHJucFg2WWorbk1URjRLaDArQUhR?=
 =?utf-8?B?dWxJTU1vWUM4RjZzVmVMQm5vem8xTFJhVzBlQVZPVW9RcTU0V1YyNS9iOVFo?=
 =?utf-8?B?eVFEdHd6WnpEWHFYTEpoWU9Ma1NhaDBpcFNYdWFtb2g0R1JmZmpkK2cyOVl5?=
 =?utf-8?B?Vi9JcC9JN2lwQzBzNVBZUGovbEZtVG5CYXR2U0M0WVFyeWladzN4UjU2RzRz?=
 =?utf-8?B?bVR4TFJWZ2FWZVhvcUNtRk95RG5BZUtWK2xoRFRTVnlGU3pXVEwwM1doYjBZ?=
 =?utf-8?B?ODJaeDBlSTFhZEtodjVuNVIxTGFMS3F3dXhHY1JsY29KZ0RFSnFJQ1VXcnpG?=
 =?utf-8?B?ekR3VjhPOFN1OXlPNTF3eVJpT056WUZPT2p4b0JqVG4zMXhKaWE3alJwUlZM?=
 =?utf-8?B?dVN1SUcyN0w1TEdxQ0VtRGhBbGZEUUtJSnAvV3hiSC9YRFRKa0ZmazdsUnAv?=
 =?utf-8?B?NVVValN6TGQ3azNKQWVTMWZiV2NyMExhamgxR3FlQ200emYrT3lsNGJmc3RC?=
 =?utf-8?B?NXNXTmpVRm9wQWhVNFU2bVBiZTJjVlNGL2xKdGMwV00xTjRYN1NJOURtYk5s?=
 =?utf-8?B?K3g1ZDkzUmtxMnNoYXZLS2RJNURsVlJuSTNLcEFuZitZKzN3S2FFN1ZETThh?=
 =?utf-8?B?YmRDVnJ3T29jU0ZPSVFIVGRqZ3pRWWhpZlVNWUx3UHQ2aTcvZWltdlh5eWtV?=
 =?utf-8?B?WlJmQnU2WDdHWE84TytJcTlHTFRvNGxCQkJCSWdQVGI0THNISHVtTUlhSlZo?=
 =?utf-8?B?V3FHdGNkSWoxUFRpOERaUmxOclZBM29YckNRNU9Dbm40UmtkcXpaeFBEZE4w?=
 =?utf-8?B?QlY1NHAveHppRGZTSE04OC9KQ3c1QkFxdzc1YjBXMTFkR01tVTBoU1dTaWZ6?=
 =?utf-8?B?VTZhaTlZYitFRk9mWEkrc0ErN29MRm5yRTl0cm1jMjhpWTV0c0tZSWlaaUhU?=
 =?utf-8?B?aTJZMnRaQUFod3ZRaWNXRWpKcWI5eGVtaGJxc1ZCT3ZVTWVzTERxRnRDbG1p?=
 =?utf-8?B?UUV5eWRrNnNPOGtISzZMa2VCeTFSYmtucWlFZDM5V3h1OHFYVmlZYlVCNjBu?=
 =?utf-8?B?Z1dWNThyeTJwWFVVSHhXVWFVREticDlZZTA5WjdwMWxWWDBBQVAwRkViNzZq?=
 =?utf-8?B?RGRKUThFZWxnNlZ2bDJRK2J4VXhBRkp0SkZzL3orNXZyb1RmWERQekF0UTVC?=
 =?utf-8?B?MzFNUHRoQkllMi8yUzN1aGlmalNTMlIySWdQMDFDbG1rdDRlUlo5dEY1RjRY?=
 =?utf-8?B?cWk1Uk0zV1J2YlZRUmNyMjA2bHMxQjZxblVmYU5sMWZNbGw3YTJkTjM1YUhQ?=
 =?utf-8?B?STdVZXJXWWo1Qkh2MnlFWHhrU2tKTCtGOUlQMmpGTE5CYUR3WnlBSHhrZi9X?=
 =?utf-8?B?MTZ6cVpKcGdFeThSRkpMNDVJNFd1OFpzM0oydzdTeVIrT1VOeHRpcTFXdTJO?=
 =?utf-8?B?elRCMnlaQWgyanQvbzM1WUhqOGdkNDBtdEJWdGZSbmJxTFgvWlFEb0NpWVdl?=
 =?utf-8?B?d0V4VGxCK3JsSTR4aGRSZEpRY2hoRksrSEViSlBwNlRBMks3NGs0QWliSEpY?=
 =?utf-8?B?Y2dRa0RnMGtKdXowb2VaV1R1b1VaR1A4SEZlKy9LRE5uRmhteGpQdlZzY0hh?=
 =?utf-8?B?MmlEUVQ0L1ljdkk4Z2szcy8zbnp5cWlQL0xnQzBYSTdvM1pZQkpMc282T0xz?=
 =?utf-8?B?em5JeHlHQjM1cEQxa0VselVMREt0MGFSaXpMR2Rxd1JOWXZRNnI5Q01Ra2tk?=
 =?utf-8?B?OVQ4Rm1iUHMzSmQyNWgrZE1EOHJkYkdpU3dtQWFzazlIajhDcnNJMjE4ai9j?=
 =?utf-8?B?RVBTYWJUMVpEbGNjTVRrQzFEUTd1N0I0WG01a1E2SVA2MlNPSG91cE5vNTVZ?=
 =?utf-8?B?M3owSE16RHdtMHRuT1p6UThhY1paNyt5SFEwR2p3bUxnVUErUmdkREVuNU05?=
 =?utf-8?Q?TTnFbSPt8EPDAOBsfbWecVndu?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e3cc490e-417d-4b86-b84d-08db36498d96
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Apr 2023 02:49:34.0862
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: XYFR3RwqVBQPFU8HApfP8aKkYwYoMbBZpg+mpCYcE5L1wG/XCZxBTtrQL76TShrqghRk2V5zmnj212edV0cPgA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8183
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE
        autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?
If yes I can change the code accordingly, otherwise I can change the "safe"
to "exclusive" and add some comments.

thanks,
Chenyu