| Message ID | 20240522074658.2420468-3-Sukrit.Bhatnagar@sony.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Improve dmesg output for swapfile+hibernation | expand |
On Wed, May 22, 2024 at 04:46:58PM +0900, Sukrit Bhatnagar wrote: > When a swapfile is created for hibernation purposes, we always need > the starting physical block offset, which is usually determined using > userspace commands such as filefrag. If you always need this value, then shouldn't it be exported via sysfs or somewhere so that you can always get to it? The kernel ringbuffer can overwrite log messages, swapfiles can get disabled, etc. > It would be good to have that value printed when we do swapon and get > that value directly from dmesg. > > Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> > --- > mm/swapfile.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/swapfile.c b/mm/swapfile.c > index f6ca215fb92f..53c9187d5fbe 100644 > --- a/mm/swapfile.c > +++ b/mm/swapfile.c > @@ -3264,8 +3264,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags) > (swap_flags & SWAP_FLAG_PRIO_MASK) >> SWAP_FLAG_PRIO_SHIFT; > enable_swap_info(p, prio, swap_map, cluster_info); > > - pr_info("Adding %uk swap on %s. Priority:%d extents:%d across:%lluk %s%s%s%s\n", > + pr_info("Adding %uk swap on %s. Priority:%d extents:%d start:%llu across:%lluk %s%s%s%s\n", > K(p->pages), name->name, p->prio, nr_extents, > + (unsigned long long)first_se(p)->start_block, Last time I looked, start_block was in units of PAGE_SIZE, despite add_swap_extent confusingly (ab)using the sector_t type. Wherever you end up reporting this value, it ought to be converted to something more common (like byte offset or 512b-block offset). Also ... if this is a swap *file* then reporting the path and the physical storage device address is not that helpful. Exposing the block device major/minor and block device address would be much more useful, wouldn't it? (Not that I have any idea what the "suspend process" in the cover letter refers to -- suspend and hibernate have been broken on xfs forever...) --D > K((unsigned long long)span), > (p->flags & SWP_SOLIDSTATE) ? "SS" : "", > (p->flags & SWP_DISCARDABLE) ? "D" : "", > -- > 2.34.1 > >
On Wed, May 22, 2024 at 07:56:37AM -0700, Darrick J. Wong wrote: > On Wed, May 22, 2024 at 04:46:58PM +0900, Sukrit Bhatnagar wrote: > > When a swapfile is created for hibernation purposes, we always need > > the starting physical block offset, which is usually determined using > > userspace commands such as filefrag. > > If you always need this value, then shouldn't it be exported via sysfs > or somewhere so that you can always get to it? The kernel ringbuffer > can overwrite log messages, swapfiles can get disabled, etc. Scraping a block address from anything is just broken. Wher is the code using this? It needs a proper kernel interface. Same about the warning crap in patch 1.
Hi Darrick, On 2024-05-22 23:56, Darrick Wong wrote: > On Wed, May 22, 2024 at 04:46:58PM +0900, Sukrit Bhatnagar wrote: >> When a swapfile is created for hibernation purposes, we always need >> the starting physical block offset, which is usually determined using >> userspace commands such as filefrag. > > If you always need this value, then shouldn't it be exported via sysfs > or somewhere so that you can always get to it? The kernel ringbuffer > can overwrite log messages, swapfiles can get disabled, etc. I agree on using appropriate kernel interfaces instead of kernel log. >> It would be good to have that value printed when we do swapon and get >> that value directly from dmesg. >> >> Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> >> --- >> mm/swapfile.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> diff --git a/mm/swapfile.c b/mm/swapfile.c >> index f6ca215fb92f..53c9187d5fbe 100644 >> --- a/mm/swapfile.c >> +++ b/mm/swapfile.c >> @@ -3264,8 +3264,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags) >> (swap_flags & SWAP_FLAG_PRIO_MASK) >> SWAP_FLAG_PRIO_SHIFT; >> enable_swap_info(p, prio, swap_map, cluster_info); >> - pr_info("Adding %uk swap on %s. Priority:%d extents:%d across:%lluk %s%s%s%s\n", >> + pr_info("Adding %uk swap on %s. Priority:%d extents:%d start:%llu across:%lluk %s%s%s%s\n", >> K(p->pages), name->name, p->prio, nr_extents, >> + (unsigned long long)first_se(p)->start_block, > > Last time I looked, start_block was in units of PAGE_SIZE, despite > add_swap_extent confusingly (ab)using the sector_t type. Wherever you > end up reporting this value, it ought to be converted to something more > common (like byte offset or 512b-block offset). I could not find any swap-related entries in the sysfs, but there is /proc/swaps which shows the enabled swaps in a table. A column for this start offset could be added there, which as you have mentioned, should be in a unit such as bytes instead of PAGE_SIZE blocks. > Also ... if this is a swap *file* then reporting the path and the > physical storage device address is not that helpful. Exposing the block > device major/minor and block device address would be much more useful, > wouldn't it? For exposing information about swap file path, I think it wouldn't make much difference (at least for the hibernate case) as we can always do the file-path -> bdev-path -> major:minor conversion in userspace. > (Not that I have any idea what the "suspend process" in the cover letter > refers to -- suspend and hibernate have been broken on xfs forever...) By suspend process, I meant the series of steps taken when we trigger hibernate's suspend-to-disk. Not the task that started it. (Wrong choice of words, my bad). -- Sukrit
diff --git a/mm/swapfile.c b/mm/swapfile.c index f6ca215fb92f..53c9187d5fbe 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3264,8 +3264,9 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags) (swap_flags & SWAP_FLAG_PRIO_MASK) >> SWAP_FLAG_PRIO_SHIFT; enable_swap_info(p, prio, swap_map, cluster_info); - pr_info("Adding %uk swap on %s. Priority:%d extents:%d across:%lluk %s%s%s%s\n", + pr_info("Adding %uk swap on %s. Priority:%d extents:%d start:%llu across:%lluk %s%s%s%s\n", K(p->pages), name->name, p->prio, nr_extents, + (unsigned long long)first_se(p)->start_block, K((unsigned long long)span), (p->flags & SWP_SOLIDSTATE) ? "SS" : "", (p->flags & SWP_DISCARDABLE) ? "D" : "",
When a swapfile is created for hibernation purposes, we always need the starting physical block offset, which is usually determined using userspace commands such as filefrag. It would be good to have that value printed when we do swapon and get that value directly from dmesg. Signed-off-by: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com> --- mm/swapfile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)