[PATCH] Skip non-regular files in recursive defrag

[PATCH] Skip non-regular files in recursive defrag

[Pascal VITOUX]

---
 cmds-filesystem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 1c1926b..979dbd9 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -646,7 +646,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
 	int e = 0;
 	int fd = 0;
 
-	if (typeflag == FTW_F) {
+	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
 		if (defrag_global_verbose)
 			printf("%s\n", fpath);
 		fd = open(fpath, O_RDWR);
-- 
1.8.5.2

Re: [PATCH] Skip non-regular files in recursive defrag

[David Sterba]

On Wed, Jan 01, 2014 at 03:10:25PM +0100, Pascal VITOUX wrote:
> ---
>  cmds-filesystem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> index 1c1926b..979dbd9 100644
> --- a/cmds-filesystem.c
> +++ b/cmds-filesystem.c
> @@ -646,7 +646,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
>  	int e = 0;
>  	int fd = 0;
>  
> -	if (typeflag == FTW_F) {
> +	if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {

The ftw(3) documentation states that FTW_F gets the regular files, do you have a
reproducer whre this does not work as expected?

I think the check could be placed into cmd_defrag, there's a st_mode
check for directories already. Defragmenting makes most sense for
regular files (though there's the option to defrag portions of the
metadata b-trees), so it's fine to handle just directories and regular
files in userspace.

If you're going to resend the patch, please add 'btrfs-progs: ' string
to the subject, write a short changlog and add your Signed-off-by line.

thanks,
david

Re: [PATCH] Skip non-regular files in recursive defrag

[Pascal VITOUX]

On Mon, Jan 6, 2014 at 4:36 PM, David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Jan 01, 2014 at 03:10:25PM +0100, Pascal VITOUX wrote:
> > ---
> >  cmds-filesystem.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/cmds-filesystem.c b/cmds-filesystem.c
> > index 1c1926b..979dbd9 100644
> > --- a/cmds-filesystem.c
> > +++ b/cmds-filesystem.c
> > @@ -646,7 +646,7 @@ static int defrag_callback(const char *fpath, const struct stat *sb,
> >       int e = 0;
> >       int fd = 0;
> >
> > -     if (typeflag == FTW_F) {
> > +     if ((typeflag == FTW_F) && S_ISREG(sb->st_mode)) {
>
> The ftw(3) documentation states that FTW_F gets the regular files, do you have a
> reproducer whre this does not work as expected?

Yes, this can be tested with the example code provided in ftw(3) and
compiled with glibc 2.18.

In fact, the ftw(3) page also states in the NOTES part that, under
Linux, FTW_F is used for all objects that are not a directory. Some
libc versions are specified ( libc4, libc5 and glic 2.0.6 ) but it's
still true in glibc 2.18

The ftw's man page is less precise than the libc manual, as you can
see here : http://www.gnu.org/software/libc/manual/html_node/Working-with-Directory-Trees.html

> I think the check could be placed into cmd_defrag, there's a st_mode
> check for directories already. Defragmenting makes most sense for
> regular files (though there's the option to defrag portions of the
> metadata b-trees), so it's fine to handle just directories and regular
> files in userspace.

I think so, too. And for a single file defrag the check should return
to the user an explicit error message instead of the 'defrag range
ioctl not supported'  error.

> If you're going to resend the patch, please add 'btrfs-progs: ' string
> to the subject, write a short changlog and add your Signed-off-by line.
>
> thanks,
> david

Ok, thanks for these instructions. I will resend it soon.

Pascal.