Re: [PATCH 2/3] xfs: set firstfsb to NULLFSBLOCK before feeding it to _bmapi_write

[Brian Foster <bfoster@redhat.com>]

On Tue, Jul 18, 2017 at 11:24:22AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We must initialize the firstfsb parameter to _bmapi_write so that it
> doesn't incorrectly treat stack garbage as a restriction on which AGs
> it can search for free space.
> 
> Fixes-coverity-id: 1402025
> Fixes-coverity-id: 1415167
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap.c |    2 +-
>  fs/xfs/xfs_reflink.c     |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 935adde..8c4ee60 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6508,7 +6508,7 @@ xfs_bmap_finish_one(
>  	xfs_filblks_t			*blockcount,
>  	xfs_exntst_t			state)
>  {
> -	xfs_fsblock_t			firstfsb;
> +	xfs_fsblock_t			firstfsb = NULLFSBLOCK;
>  	int				error = 0;
>  

Shouldn't firstfsb be tied to the transaction lifetime in (at least)
this case? It is used in the allocator to control things like AG locking
order. Here, it looks like we could potentially do multiple unmaps in a
single transaction without carrying the firstfsb state across..?

FWIW this patch still looks fine either way:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  	trace_xfs_bmap_deferred(tp->t_mountp,
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index ab2270a..d9b3d57 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -329,7 +329,7 @@ xfs_reflink_convert_cow_extent(
>  	xfs_filblks_t			count_fsb,
>  	struct xfs_defer_ops		*dfops)
>  {
> -	xfs_fsblock_t			first_block;
> +	xfs_fsblock_t			first_block = NULLFSBLOCK;
>  	int				nimaps = 1;
>  
>  	if (imap->br_state == XFS_EXT_NORM)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html