| Message ID | 20240522074658.2420468-2-Sukrit.Bhatnagar@sony.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Improve dmesg output for swapfile+hibernation | expand |
Hi Sukrit, It seems that you need the swap file block start address to read the swap file headers. This warning still requires the user to read the dmesg. The kernel still does not have the swapfile header at resume. In other words, it does not fix the issue. I don't know the suspend/resume code enough, will adding recording the physical start address of the swapfile in swap_info_struct help you address this problem? The suspend code can write that value to "somewhere* for resume to pick it up. Let's find a proper way to fix this issue rather than just warning on it. Chris On Wed, May 22, 2024 at 12:42 AM Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> wrote: > > When creating a swapfile on a filesystem with block size less than the > PAGE_SIZE, there is a possibility that the starting physical block is not > page-aligned, which results in rounding up that value before setting it > in the first swap extent. But now that the value is rounded up, we have > lost the actual offset location of the first physical block. > > The starting physical block value is needed in hibernation when using a > swapfile, i.e., the resume_offset. After we have written the snapshot > pages, some values will be set in the swap header which is accessed using > that offset location. However, it will not find the swap header if the > offset value was rounded up and results in an error. > > The swapfile offset being unaligned should not fail the swapon activation > as the swap extents will always have the alignment. > > Therefore, just print a warning if an unaligned swapfile is activated > when hibernation is enabled. > > Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> > --- > fs/iomap/swapfile.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/fs/iomap/swapfile.c b/fs/iomap/swapfile.c > index 5fc0ac36dee3..1f7b189089dd 100644 > --- a/fs/iomap/swapfile.c > +++ b/fs/iomap/swapfile.c > @@ -49,6 +49,16 @@ static int iomap_swapfile_add_extent(struct iomap_swapfile_info *isi) > next_ppage = ALIGN_DOWN(iomap->addr + iomap->length, PAGE_SIZE) >> > PAGE_SHIFT; > > +#ifdef CONFIG_HIBERNATION > + /* > + * Print a warning if the starting physical block is not aligned > + * to PAGE_SIZE (for filesystems using smaller block sizes). > + * This will fail the hibernation suspend as we need to read > + * the swap header later using the starting block offset. > + */ > + if (!iomap->offset && iomap->addr & PAGE_MASK) > + pr_warn("swapon: starting physical offset not page-aligned\n"); > +#endif > /* Skip too-short physical extents. */ > if (first_ppage >= next_ppage) > return 0; > -- > 2.34.1 > >
Hi Chris, On 2024-05-23 03:02, Chris Li wrote: > Hi Sukrit, > > It seems that you need the swap file block start address to read the > swap file headers. > This warning still requires the user to read the dmesg. The kernel > still does not have the swapfile header at resume. In other words, it > does not fix the issue. This was not intended to be a fix. I had created this patch thinking that adding an actual fix for this might be non-trivial and may not be desirable to everyone, especially when swapfile+hibernation is not commonly used. > I don't know the suspend/resume code enough, will adding recording the > physical start address of the swapfile in swap_info_struct help you > address this problem? The suspend code can write that value to > "somewhere* for resume to pick it up. > > Let's find a proper way to fix this issue rather than just warning on it. Adding a new member in swap_info_struct for the physical block offset will help in the fix, I think. It can even be enclosed in #ifdef HIBERNATION if nothing else needs that value. This value can be checked by hibernate[1] as: sis->start_offset == first_se(sis)->start_block Another possible solution might be to add a new swap flag like SWP_SUSP or SWP_HIBERNATE and set it only when we don't have this rounding up issue. Since we will boot the kernel normally first and later switch to our pre-hibernate kernel after image load from swap, the value cannot be set inside the kernel memory. We pass the start block address in kernel commandline parameter (resume_offset) for next boot. [1]: The hibernate swapfile checking code is the function swap_type_of() in mm/swapfile.c -- Sukrit
diff --git a/fs/iomap/swapfile.c b/fs/iomap/swapfile.c index 5fc0ac36dee3..1f7b189089dd 100644 --- a/fs/iomap/swapfile.c +++ b/fs/iomap/swapfile.c @@ -49,6 +49,16 @@ static int iomap_swapfile_add_extent(struct iomap_swapfile_info *isi) next_ppage = ALIGN_DOWN(iomap->addr + iomap->length, PAGE_SIZE) >> PAGE_SHIFT; +#ifdef CONFIG_HIBERNATION + /* + * Print a warning if the starting physical block is not aligned + * to PAGE_SIZE (for filesystems using smaller block sizes). + * This will fail the hibernation suspend as we need to read + * the swap header later using the starting block offset. + */ + if (!iomap->offset && iomap->addr & PAGE_MASK) + pr_warn("swapon: starting physical offset not page-aligned\n"); +#endif /* Skip too-short physical extents. */ if (first_ppage >= next_ppage) return 0;
When creating a swapfile on a filesystem with block size less than the PAGE_SIZE, there is a possibility that the starting physical block is not page-aligned, which results in rounding up that value before setting it in the first swap extent. But now that the value is rounded up, we have lost the actual offset location of the first physical block. The starting physical block value is needed in hibernation when using a swapfile, i.e., the resume_offset. After we have written the snapshot pages, some values will be set in the swap header which is accessed using that offset location. However, it will not find the swap header if the offset value was rounded up and results in an error. The swapfile offset being unaligned should not fail the swapon activation as the swap extents will always have the alignment. Therefore, just print a warning if an unaligned swapfile is activated when hibernation is enabled. Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> --- fs/iomap/swapfile.c | 10 ++++++++++ 1 file changed, 10 insertions(+)