Re: [PATCH 7/9] xfs_spaceman: Free space mapping command

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, May 30, 2017 at 01:56:39PM -0500, Eric Sandeen wrote:
> On 5/30/17 1:40 PM, Darrick J. Wong wrote:
> > On Fri, May 26, 2017 at 08:57:14PM -0500, Eric Sandeen wrote:
> >>
> >>
> >> On 5/7/17 10:57 AM, Darrick J. Wong wrote:
> >>> From: Dave Chinner <dchinner@redhat.com>
> >>>
> >>> Add freespace mapping tool modelled on the xfs_db freesp command.
> >>> The advantage of this command over xfs_db is that it can be done
> >>> online and is coherent with concurrent modifications to the
> >>> filesystem.
> >>>
> >>> This requires the kernel to support the XFS_IOC_GETFSMAP ioctl to map
> >>> free space indexes.
> >>>
> >>> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> >>> [darrick: port from FIEMAPFS to GETFSMAP]
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>> ---
> >>>  spaceman/Makefile   |   12 +-
> >>>  spaceman/ag.c       |    1 
> >>>  spaceman/file.c     |   18 ++-
> >>>  spaceman/freesp.c   |  367 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  spaceman/init.c     |    9 +
> >>>  spaceman/prealloc.c |    1 
> >>>  spaceman/space.h    |   12 +-
> >>>  spaceman/trim.c     |    1 
> >>>  8 files changed, 413 insertions(+), 8 deletions(-)
> >>>  create mode 100644 spaceman/freesp.c
> >>>
> >>>
> >>> diff --git a/spaceman/Makefile b/spaceman/Makefile
> >>> index 08709b3..3b059ca 100644
> >>> --- a/spaceman/Makefile
> >>> +++ b/spaceman/Makefile
> >>> @@ -7,8 +7,12 @@ include $(TOPDIR)/include/builddefs
> >>>  
> >>>  LTCOMMAND = xfs_spaceman
> >>>  HFILES = init.h space.h
> >>> -CFILES = init.c \
> >>> -	ag.c file.c prealloc.c trim.c
> >>> +CFILES = ag.c \
> >>> +	 file.c \
> >>> +	 init.c \
> >>> +	 prealloc.c \
> >>> +	 trim.c
> >>> +
> >>>  
> >>>  LLDLIBS = $(LIBXCMD)
> >>>  LTDEPENDENCIES = $(LIBXCMD)
> >>> @@ -22,6 +26,10 @@ ifeq ($(ENABLE_EDITLINE),yes)
> >>>  LLDLIBS += $(LIBEDITLINE) $(LIBTERMCAP)
> >>>  endif
> >>>  
> >>> +ifeq ($(HAVE_GETFSMAP),yes)
> >>> +CFILES += freesp.c
> >>> +endif
> >>> +
> >>>  default: depend $(LTCOMMAND)
> >>>  
> >>>  include $(BUILDRULES)
> >>> diff --git a/spaceman/ag.c b/spaceman/ag.c
> >>> index 1eb8aa0..0f1c869 100644
> >>> --- a/spaceman/ag.c
> >>> +++ b/spaceman/ag.c
> >>> @@ -21,6 +21,7 @@
> >>>  #include "command.h"
> >>>  #include "input.h"
> >>>  #include "init.h"
> >>> +#include "path.h"
> >>>  #include "space.h"
> >>>  
> >>>  #ifndef XFS_IOC_AGCONTROL
> >>> diff --git a/spaceman/file.c b/spaceman/file.c
> >>> index 9356066..7c5ea0e 100644
> >>> --- a/spaceman/file.c
> >>> +++ b/spaceman/file.c
> >>> @@ -22,6 +22,7 @@
> >>>  #include "command.h"
> >>>  #include "input.h"
> >>>  #include "init.h"
> >>> +#include "path.h"
> >>>  #include "space.h"
> >>>  
> >>>  static cmdinfo_t print_cmd;
> >>> @@ -69,8 +70,10 @@ openfile(
> >>>  	char		*path,
> >>>  	xfs_fsop_geom_t	*geom,
> >>>  	int		flags,
> >>> -	mode_t		mode)
> >>> +	mode_t		mode,
> >>> +	struct fs_path	*fs_path)
> >>>  {
> >>> +	struct fs_path	*fsp;
> >>>  	int		fd;
> >>>  
> >>>  	fd = open(path, flags, mode);
> >>> @@ -95,6 +98,15 @@ openfile(
> >>>  		close(fd);
> >>>  		return -1;
> >>>  	}
> >>> +
> >>> +	if (fs_path) {
> >>> +		fsp = fs_table_lookup(path, FS_MOUNT_POINT);
> >>> +		if (!fsp) {
> >>> +			fprintf(stderr, _("Unable to find XFS information."));
> >>
> >> If I got that error I wouldn't know what it meant...
> > 
> > "Unable to find mount information." ?
> 
> I think that if the path isn't found in the table, that means it wasn't
> populated on startup, which means it's not [on] an XFS filesystem:
> 
>         fs = fs_table_lookup(argv[optind], FS_MOUNT_POINT);
>         if (!fs) {
>                 fprintf(stderr, _("%s: %s is not a mounted XFS filesystem\n"),
>                         progname, argv[optind]);
>                 return 1;
>         }
> 
> or
> 
>                 fprintf(stderr, _("file argument, \"%s\", is not in a mounted XFS filesystem\n"),
>                         file->name);
> 
> is what other tools do (although quota does:
> 
>                 fprintf(stderr, "%s: unknown mount point %s\n", progname, dir);
> or
>                 fprintf(stderr, "%s: cannot find mount point %s\n",)
> 
> 
> Anyway: I think the failure means that the path is not [on] an xfs filesystem,
> so that's probably the best information to convey.

I like the second option better, so I'll change it to that.

--D

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