f2fs: don't reserve block for empty file when convert inline, page

PATCH In review Kinglong Mee 3 participants 4 messages Jan 2, 2017 – Jan 4, 2017

Subsystems (2)

VFS F2FS file system

lore

Share & tools

lore

Kinglong Mee

author

2017-01-02 14:56 UTC #0 lore raw

PATCH 1/1 f2fs: don't reserve block for empty file when convert inline, page

A test program gets the SEEK_DATA with two values between a new created file and the exist file on f2fs filesystem.

F2FS filesystem, (the first "test1" is a new file)

# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 8192)
# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 4096)

PNFS filesystem, (the first "test1" is a new file)

# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)
# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)

# cat lseektest.c

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#ifndef SEEK_DATA
#define SEEK_DATA 3
#define SEEK_HOLE 4
#endif

int main(int argc, char **argv)

{
        char *filename = argv[1];
        int offset = 1, i = 0, fd = -1;

        if (argc < 2) {
                printf("Usage: %s f2fsfilename\n", argv[0]);
                return -1;
        }

        /*
        if (!access(filename, F_OK) || errno != ENOENT) {
                printf("Needs a new file for test, %m\n");
                return -1;
        }*/

        fd = open(filename, O_RDWR | O_CREAT, 0777);
        if (fd < 0) {
                printf("Create test file %s failed, %m\n", filename);
                return -1;
        }

        for (i = 0; i < 20; i++) {
                offset = 1 << i;
                ftruncate(fd, 0);
                lseek(fd, offset, SEEK_SET);
                write(fd, "test", 5);
                /* Get the alloc size by seek data equal zero*/
                if (lseek(fd, 0, SEEK_DATA)) {
                        printf("SEEK_DATA size != 0 (offset = %d)\n", offset);
                        break;
                }
        }

        close(fd);
        return 0;
}

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/f2fs/inline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/inline.c +1 -1

index e32a9e5..6c8d099 100644
@@ -117,7 +117,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	};
 	int dirty, err;
 
-	if (!f2fs_exist_data(dn->inode))
+	if (!f2fs_exist_data(dn->inode) || !i_size_read(dn->inode))
 		goto clear_out;
 
 	err = f2fs_reserve_block(dn, 0);

Signature · 1 line

2.9.3

Jaegeuk Kim

maintainer

2017-01-04 01:23 UTC #1 lore raw

Hi Kinglong,

Quoted 79 lines · Kinglong Mee

A test program gets the SEEK_DATA with two values between a new created file and the exist file on f2fs filesystem.

F2FS filesystem, (the first "test1" is a new file)

# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 8192)
# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 4096)

PNFS filesystem, (the first "test1" is a new file)

# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)
# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)

# cat lseektest.c

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#ifndef SEEK_DATA
#define SEEK_DATA 3
#define SEEK_HOLE 4
#endif

int main(int argc, char **argv)

{
        char *filename = argv[1];
        int offset = 1, i = 0, fd = -1;

        if (argc < 2) {
                printf("Usage: %s f2fsfilename\n", argv[0]);
                return -1;
        }

        /*
        if (!access(filename, F_OK) || errno != ENOENT) {
                printf("Needs a new file for test, %m\n");
                return -1;
        }*/

        fd = open(filename, O_RDWR | O_CREAT, 0777);
        if (fd < 0) {
                printf("Create test file %s failed, %m\n", filename);
                return -1;
        }

        for (i = 0; i < 20; i++) {
                offset = 1 << i;
                ftruncate(fd, 0);
                lseek(fd, offset, SEEK_SET);
                write(fd, "test", 5);
                /* Get the alloc size by seek data equal zero*/
                if (lseek(fd, 0, SEEK_DATA)) {
                        printf("SEEK_DATA size != 0 (offset = %d)\n", offset);
                        break;
                }
        }

        close(fd);
        return 0;
}

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/f2fs/inline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/inline.c +0 -0

index e32a9e5..6c8d099 100644

fs/f2fs/inline.c int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page) · line 121

Quoted 6 lines

fs/f2fs/inline.c +1 -1

@@ -117,7 +117,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	};
 	int dirty, err;
 
-	if (!f2fs_exist_data(dn->inode))
+	if (!f2fs_exist_data(dn->inode) || !i_size_read(dn->inode))
 		goto clear_out;

The point is that f2fs_exist_data() should return true, since the file was truncated to 0. So, could you check this out?

Thanks,

---
 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

fs/f2fs/file.c +2 -0

index 826fefc05fb1..ba2021e01c33 100644
@@ -570,6 +570,8 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (f2fs_has_inline_data(inode)) {
 		if (truncate_inline_inode(ipage, from))
 			set_page_dirty(ipage);
+		if (from == 0)
+			clear_inode_flag(inode, FI_DATA_EXIST);
 		f2fs_put_page(ipage, 1);
 		truncate_page = true;
 		goto out;

Signature · 5 lines

2.11.0

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most 
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Kinglong Mee

author Tested-by

2017-01-04 08:27 UTC #2 lore raw

Quoted 85 lines · Jaegeuk Kim

Hi Kinglong,

Quoted 82 lines · Kinglong Mee

A test program gets the SEEK_DATA with two values between a new created file and the exist file on f2fs filesystem.

F2FS filesystem, (the first "test1" is a new file)

# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 8192)
# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 4096)

PNFS filesystem, (the first "test1" is a new file)

# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)
# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)

# cat lseektest.c

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#ifndef SEEK_DATA
#define SEEK_DATA 3
#define SEEK_HOLE 4
#endif

int main(int argc, char **argv)

{
        char *filename = argv[1];
        int offset = 1, i = 0, fd = -1;

        if (argc < 2) {
                printf("Usage: %s f2fsfilename\n", argv[0]);
                return -1;
        }

        /*
        if (!access(filename, F_OK) || errno != ENOENT) {
                printf("Needs a new file for test, %m\n");
                return -1;
        }*/

        fd = open(filename, O_RDWR | O_CREAT, 0777);
        if (fd < 0) {
                printf("Create test file %s failed, %m\n", filename);
                return -1;
        }

        for (i = 0; i < 20; i++) {
                offset = 1 << i;
                ftruncate(fd, 0);
                lseek(fd, offset, SEEK_SET);
                write(fd, "test", 5);
                /* Get the alloc size by seek data equal zero*/
                if (lseek(fd, 0, SEEK_DATA)) {
                        printf("SEEK_DATA size != 0 (offset = %d)\n", offset);
                        break;
                }
        }

        close(fd);
        return 0;
}

Signed-off-by Kinglong Mee <[email protected]>

---
 fs/f2fs/inline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/f2fs/inline.c +0 -0

index e32a9e5..6c8d099 100644
@@ -117,7 +117,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	};
 	int dirty, err;

Quoted 6 lines

fs/f2fs/inline.c int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page) · line 121
Quoted 3 lines
fs/f2fs/inline.c +1 -1
-	if (!f2fs_exist_data(dn->inode))
+	if (!f2fs_exist_data(dn->inode) || !i_size_read(dn->inode))
 		goto clear_out;
The point is that f2fs_exist_data() should return true, since the file was truncated to 0. So, could you check this out?

Yes, that's right.
This patch is useful.

Tested-by Kinglong Mee <[email protected]>

thanks,
Kinglong Mee

Quoted 21 lines

Thanks,

---
 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

fs/f2fs/file.c +2 -0

index 826fefc05fb1..ba2021e01c33 100644
@@ -570,6 +570,8 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (f2fs_has_inline_data(inode)) {
 		if (truncate_inline_inode(ipage, from))
 			set_page_dirty(ipage);
+		if (from == 0)
+			clear_inode_flag(inode, FI_DATA_EXIST);
 		f2fs_put_page(ipage, 1);
 		truncate_page = true;
 		goto out;

Chao Yu

maintainer Reviewed-by

2017-01-04 16:03 UTC #3 lore raw

Quoted 93 lines · Kinglong Mee

Quoted 85 lines · Jaegeuk Kim
Hi Kinglong,
Quoted 82 lines · Kinglong Mee
A test program gets the SEEK_DATA with two values between a new created file and the exist file on f2fs filesystem.
F2FS filesystem, (the first "test1" is a new file)
# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 8192)
# ./lseektest /f2fs/test1
SEEK_DATA size != 0 (offset = 4096)
PNFS filesystem, (the first "test1" is a new file)
# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)
# ./lseektest /pnfs/test1
SEEK_DATA size != 0 (offset = 4096)
# cat lseektest.c
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <unistd.h>
#include <errno.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#ifndef SEEK_DATA
#define SEEK_DATA 3
#define SEEK_HOLE 4
#endif
int main(int argc, char **argv)
{
        char *filename = argv[1];
        int offset = 1, i = 0, fd = -1;

        if (argc < 2) {
                printf("Usage: %s f2fsfilename\n", argv[0]);
                return -1;
        }

        /*
        if (!access(filename, F_OK) || errno != ENOENT) {
                printf("Needs a new file for test, %m\n");
                return -1;
        }*/

        fd = open(filename, O_RDWR | O_CREAT, 0777);
        if (fd < 0) {
                printf("Create test file %s failed, %m\n", filename);
                return -1;
        }

        for (i = 0; i < 20; i++) {
                offset = 1 << i;
                ftruncate(fd, 0);
                lseek(fd, offset, SEEK_SET);
                write(fd, "test", 5);
                /* Get the alloc size by seek data equal zero*/
                if (lseek(fd, 0, SEEK_DATA)) {
                        printf("SEEK_DATA size != 0 (offset = %d)\n", offset);
                        break;
                }
        }

        close(fd);
        return 0;
}
Signed-off-by Kinglong Mee <[email protected]>
---
 fs/f2fs/inline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/f2fs/inline.c +0 -0
index e32a9e5..6c8d099 100644
@@ -117,7 +117,7 @@ int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page)
 	};
 	int dirty, err;
 
Quoted 6 lines
fs/f2fs/inline.c int f2fs_convert_inline_page(struct dnode_of_data *dn, struct page *page) · line 121
Quoted 3 lines
fs/f2fs/inline.c +1 -1
-	if (!f2fs_exist_data(dn->inode))
+	if (!f2fs_exist_data(dn->inode) || !i_size_read(dn->inode))
 		goto clear_out;
The point is that f2fs_exist_data() should return true, since the file was truncated to 0. So, could you check this out?

Quoted 4 lines

Yes, that's right.
This patch is useful.
Tested-by Kinglong Mee <[email protected]>

Reviewed-by Chao Yu <[email protected]>

Thanks,

Quoted 26 lines

thanks,
Kinglong Mee

Quoted 13 lines

Thanks,

---
 fs/f2fs/file.c | 2 ++
 1 file changed, 2 insertions(+)

fs/f2fs/file.c +0 -0

index 826fefc05fb1..ba2021e01c33 100644

fs/f2fs/file.c int truncate_blocks(struct inode *inode, u64 from, bool lock) · line 577

Quoted 8 lines

fs/f2fs/file.c +2 -0

@@ -570,6 +570,8 @@ int truncate_blocks(struct inode *inode, u64 from, bool lock)
 	if (f2fs_has_inline_data(inode)) {
 		if (truncate_inline_inode(ipage, from))
 			set_page_dirty(ipage);
+		if (from == 0)
+			clear_inode_flag(inode, FI_DATA_EXIST);
 		f2fs_put_page(ipage, 1);
 		truncate_page = true;
 		goto out;

Quoted 8 lines

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot