Received-SPF: pass (google.com: domain of linux-kernel+bounces-186392-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 22 May 2024 07:56:37 -0700
From: "Darrick J. Wong" <djwong@kernel.org>
To: Sukrit Bhatnagar <Sukrit.Bhatnagar@sony.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Pavel Machek <pavel@ucw.cz>,
	Christian Brauner <brauner@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-xfs@vger.kernel.org, linux-pm@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH 2/2] mm: swap: print starting physical block offset in
 swapon
Message-ID: <20240522145637.GV25518@frogsfrogsfrogs>
References: <20240522074658.2420468-1-Sukrit.Bhatnagar@sony.com>
 <20240522074658.2420468-3-Sukrit.Bhatnagar@sony.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240522074658.2420468-3-Sukrit.Bhatnagar@sony.com>

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
> 
>