Re: [PATCH] xfs: Fix 'set but not used' warning in xfs_bmap_compute_alignments()

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Jan 29, 2021 at 07:46:38AM -0500, Brian Foster wrote:
> On Fri, Jan 29, 2021 at 01:10:00PM +0530, Chandan Babu R wrote:
> > On 28 Jan 2021 at 23:14, Darrick J. Wong wrote:
> > > On Thu, Jan 28, 2021 at 10:34:12AM -0500, Brian Foster wrote:
> > >> On Wed, Jan 27, 2021 at 02:35:37PM +0530, Chandan Babu R wrote:
> > >> > With both CONFIG_XFS_DEBUG and CONFIG_XFS_WARN disabled, the only reference to
> > >> > local variable "error" in xfs_bmap_compute_alignments() gets eliminated during
> > >> > pre-processing stage of the compilation process. This causes the compiler to
> > >> > generate a "set but not used" warning.
> > >> >
> > >> > Reported-by: kernel test robot <lkp@intel.com>
> > >> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > >> > ---
> > >> > This patch is applicable on top of current xfs-linux/for-next branch.
> > >> >
> > >> >  fs/xfs/libxfs/xfs_bmap.c | 9 ++++-----
> > >> >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >> >
> > >> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > >> > index 2cd24bb06040..ba56554e8c05 100644
> > >> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > >> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > >> > @@ -3471,7 +3471,6 @@ xfs_bmap_compute_alignments(
> > >> >  	struct xfs_mount	*mp = args->mp;
> > >> >  	xfs_extlen_t		align = 0; /* minimum allocation alignment */
> > >> >  	int			stripe_align = 0;
> > >> > -	int			error;
> > >> >
> > >> >  	/* stripe alignment for allocation is determined by mount parameters */
> > >> >  	if (mp->m_swidth && (mp->m_flags & XFS_MOUNT_SWALLOC))
> > >> > @@ -3484,10 +3483,10 @@ xfs_bmap_compute_alignments(
> > >> >  	else if (ap->datatype & XFS_ALLOC_USERDATA)
> > >> >  		align = xfs_get_extsz_hint(ap->ip);
> > >> >  	if (align) {
> > >> > -		error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> > >> > -						align, 0, ap->eof, 0, ap->conv,
> > >> > -						&ap->offset, &ap->length);
> > >> > -		ASSERT(!error);
> > >> > +		if (xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
> > >> > +			align, 0, ap->eof, 0, ap->conv, &ap->offset,
> > >> > +			&ap->length))
> > >> > +			ASSERT(0);
> > >>
> > >> I was wondering if we should just make xfs_bmap_extsize_align() return
> > >> void and push the asserts down into the function itself, but it looks
> > >> like xfs_bmap_rtalloc() actually handles the error. Any idea on why we
> > >> might have that inconsistency?
> > >
> > > It only returns nonzero if isrt (the fifth parameter) is nonzero, and
> > > only if the requested range is still not aligned to the rt extent size
> > > after aligning it and eliminating any overlaps with existing extents.
> > >
> > 
> > Adding to what Darrick has mentioned above ...
> > 
> > Space on realtime devices are tracked at a granularity of "rextsize"
> > bytes. Each bit held in the data blocks of xfs_mount->m_rbmip represents usage
> > status of a single rextsized block. Most likely this seems to be underlying
> > reason for strict allocation alignment requirements for realtime files.
> > 
> 
> Ah, I see. Could you fix the indentation/alignment of the call so it
> looks something like the following?
> 
> 		if (xfs_bmap_extsize_align(mp, &ap->got, &ap->prev, align, 0,
> 					   ap->eof, 0, ap->conv, &ap->offset,
> 					   &ap->length))
> 			ASSERT(0);
> 
> Otherwise the patch seems fine to me.

Yeah, I'll fix it in my tree before I push out for-next again.

--D

> Brian
> 
> > --
> > chandan
> > 
>