[bug report] xfs: scrub directory freespace

[bug report] xfs: scrub directory freespace

[Dan Carpenter]

Hello Darrick J. Wong,

The patch df481968f33b: "xfs: scrub directory freespace" from Oct 17,
2017, leads to the following static checker warning:

	fs/xfs/scrub/dir.c:446 xfs_scrub_directory_check_freesp()
	info: ignoring unreachable code.

fs/xfs/scrub/dir.c
   431  STATIC void
   432  xfs_scrub_directory_check_freesp(
   433          struct xfs_scrub_context        *sc,
   434          xfs_dablk_t                     lblk,
   435          struct xfs_buf                  *dbp,
   436          unsigned int                    len)
   437  {
   438          struct xfs_dir2_data_free       *bf;
   439          struct xfs_dir2_data_free       *dfp;
   440          int                             offset;
   441  
   442          if (len == 0)
   443                  return;
   444  
   445          bf = sc->ip->d_ops->data_bestfree_p(dbp->b_addr);
   446          for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
                                                                      ^^^^^
This looks like a loop

   447                  offset = be16_to_cpu(dfp->offset);
   448                  if (offset == 0)
   449                          break;
   450                  if (len == be16_to_cpu(dfp->length))
   451                          return;
   452                  /* Didn't find the best length in the bestfree data */
   453                  break;
                        ^^^^^^
but we always either break or return on the first iteration.  What's
going on?

   454          }
   455  
   456          xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
   457  }

regards,
dan carpenter

Re: [bug report] xfs: scrub directory freespace

[Darrick J. Wong]

On Sat, Nov 04, 2017 at 10:54:57AM +0300, Dan Carpenter wrote:
> Hello Darrick J. Wong,
> 
> The patch df481968f33b: "xfs: scrub directory freespace" from Oct 17,
> 2017, leads to the following static checker warning:
> 
> 	fs/xfs/scrub/dir.c:446 xfs_scrub_directory_check_freesp()
> 	info: ignoring unreachable code.
> 
> fs/xfs/scrub/dir.c
>    431  STATIC void
>    432  xfs_scrub_directory_check_freesp(
>    433          struct xfs_scrub_context        *sc,
>    434          xfs_dablk_t                     lblk,
>    435          struct xfs_buf                  *dbp,
>    436          unsigned int                    len)
>    437  {
>    438          struct xfs_dir2_data_free       *bf;
>    439          struct xfs_dir2_data_free       *dfp;
>    440          int                             offset;
>    441  
>    442          if (len == 0)
>    443                  return;
>    444  
>    445          bf = sc->ip->d_ops->data_bestfree_p(dbp->b_addr);
>    446          for (dfp = &bf[0]; dfp < &bf[XFS_DIR2_DATA_FD_COUNT]; dfp++) {
>                                                                       ^^^^^
> This looks like a loop
> 
>    447                  offset = be16_to_cpu(dfp->offset);
>    448                  if (offset == 0)
>    449                          break;
>    450                  if (len == be16_to_cpu(dfp->length))
>    451                          return;
>    452                  /* Didn't find the best length in the bestfree data */
>    453                  break;
>                         ^^^^^^
> but we always either break or return on the first iteration.  What's
> going on?

I think this is a broken refactoring of the bestfree checker --
previously we'd examine every slot in a directory's bestfree data block,
read the corresponding directory data block, and check that the slot
matches any of the data block's bestfree entries.  Then Dave pointed out
that the slot must match the /first/ entry in the dir data block, so we
only need to examine bf[0], and this loop can be unrolled.

Thanks for catching this!

--D

>    454          }
>    455  
>    456          xfs_scrub_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
>    457  }
> 
> regards,
> dan carpenter
> --
> 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