xfs
[Top] [All Lists]

Re: [PATCH] xfs: add FITRIM support

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: add FITRIM support
From: Alex Elder <aelder@xxxxxxx>
Date: Tue, 04 Jan 2011 15:55:15 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110102072202.GA26488@xxxxxxxxxxxxx>
References: <20110102072202.GA26488@xxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Sun, 2011-01-02 at 02:22 -0500, Christoph Hellwig wrote:
> Allow manual discards from userspace using the FITRIM ioctl.  This is not
> intended to be run during normal workloads, as the freepsace btree walks
> can cause large performance degradation.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

This looks good, but I mention three things below.
Correct them as you see fit, either way:

Reviewed-by: Alex Elder <aelder@xxxxxxx>


> ---
> 
> V1 -> V2
>  
>  - added __user annotations as noted by Alex
>  - removed non-blocking agf read as noted by Alex
>  - update range->len as noted by Alex
> 
> This does not implement the by-bno search or lock break suggestions from
> Dave.  Given that the 2.6.38 window is about to close those seem a bit
> risky to me.  I'll look into these later.

. . .

>                                  xfs_fs_subr.o \
> Index: xfs/fs/xfs/linux-2.6/xfs_discard.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ xfs/fs/xfs/linux-2.6/xfs_discard.c        2011-01-02 08:06:15.828014477 
> +0100
> @@ -0,0 +1,191 @@
> +/*
> + * Copyright (C) 2010 Red Hat, Inc.

Maybe 2011 now...

> + * All Rights Reserved.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write the Free Software Foundation,
> + * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + */
> +#include "xfs.h"
> +#include "xfs_sb.h"

. . .

> +
> +STATIC int
> +xfs_trim_extents(
> +     struct xfs_mount        *mp,
. . .
> +
> +             /*
> +              * If the extent is entirely outside of the range we are
> +              * supposed to discard skip it.  Do not bother to trim
> +              * down partially overlapping ranges for now.
> +              */
> +             if (XFS_AGB_TO_FSB(mp, agno, fbno) + flen < start ||
> +                 XFS_AGB_TO_FSB(mp, agno, fbno) > start + len) {

                                                    ^
                                          I think this should be >=

> +                     trace_xfs_discard_exclude(mp, agno, fbno, flen);
> +                     goto next_extent;
> +             }

. . .

> Index: xfs/fs/xfs/linux-2.6/xfs_discard.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ xfs/fs/xfs/linux-2.6/xfs_discard.h        2011-01-02 07:11:43.693026629 
> +0100

Do you want to add a boilerplate copyright header here?

> @@ -0,0 +1,8 @@
> +#ifndef XFS_DISCARD_H
> +#define XFS_DISCARD_H 1
> +
> +struct fstrim_range;
> +
> +extern int   xfs_ioc_trim(struct xfs_mount *, struct fstrim_range __user *);
> +
> +#endif /* XFS_DISCARD_H */

. . .


<Prev in Thread] Current Thread [Next in Thread>