2014-01-03 09:15:57

by Robert Yang

[permalink] [raw]
Subject: [PATCH 0/2] e2fsprogs: two fixes

* Fix icache
* Fix populate-extfs.sh

Robert Yang (1):
inode.c: only update the icache for ext2_inode

Søren Holm (1):
populate-extfs.sh: espace the space in the filename

contrib/populate-extfs.sh | 20 ++++++++++----------
lib/ext2fs/inode.c | 20 ++++++++++++--------
2 files changed, 22 insertions(+), 18 deletions(-)
mode change 100755 => 100644 contrib/populate-extfs.sh

--
1.7.10.4


2014-01-03 09:15:57

by Robert Yang

[permalink] [raw]
Subject: [PATCH 1/2] inode.c: only update the icache for ext2_inode

We read the cache when:

bufsize == sizeof(struct ext2_inode)

so we should update the cache in the same rule, otherwise there would be
errors, for example:

cache[0]: cached ino 14 when bufsize = 128 by ext2fs_write_inode_full()
cache[1]: cached ino 14 when bufsize = 156 by ext2fs_read_inode_full()

Then update the cache:
cache[0]: cached ino 15 when bufsize = 156 by ext2fs_read_inode_full()

And the ino 14 would hit the cache[1] when bufsize = 128 (but it was
cached by bufsize = 156), so there would be errors.

Signed-off-by: Robert Yang <[email protected]>
---
lib/ext2fs/inode.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/ext2fs/inode.c b/lib/ext2fs/inode.c
index 573a8fa..cd85b47 100644
--- a/lib/ext2fs/inode.c
+++ b/lib/ext2fs/inode.c
@@ -614,10 +614,12 @@ errcode_t ext2fs_read_inode_full(ext2_filsys fs, ext2_ino_t ino,
#endif

/* Update the inode cache */
- fs->icache->cache_last = (fs->icache->cache_last + 1) %
- fs->icache->cache_size;
- fs->icache->cache[fs->icache->cache_last].ino = ino;
- fs->icache->cache[fs->icache->cache_last].inode = *inode;
+ if (bufsize == sizeof(struct ext2_inode)) {
+ fs->icache->cache_last = (fs->icache->cache_last + 1) %
+ fs->icache->cache_size;
+ fs->icache->cache[fs->icache->cache_last].ino = ino;
+ fs->icache->cache[fs->icache->cache_last].inode = *inode;
+ }

return 0;
}
@@ -650,10 +652,12 @@ errcode_t ext2fs_write_inode_full(ext2_filsys fs, ext2_ino_t ino,

/* Check to see if the inode cache needs to be updated */
if (fs->icache) {
- for (i=0; i < fs->icache->cache_size; i++) {
- if (fs->icache->cache[i].ino == ino) {
- fs->icache->cache[i].inode = *inode;
- break;
+ if (bufsize == sizeof(struct ext2_inode)) {
+ for (i=0; i < fs->icache->cache_size; i++) {
+ if (fs->icache->cache[i].ino == ino) {
+ fs->icache->cache[i].inode = *inode;
+ break;
+ }
}
}
} else {
--
1.7.10.4


2014-01-03 09:15:57

by Robert Yang

[permalink] [raw]
Subject: [PATCH 2/2] populate-extfs.sh: espace the space in the filename

From: Søren Holm <[email protected]>

The filename which contains space would not get into the final
ext2/3/4 filsystem without this patch

Signed-off-by: Søren Holm <[email protected]>
Signed-off-by: Robert Yang <[email protected]>
---
contrib/populate-extfs.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
mode change 100755 => 100644 contrib/populate-extfs.sh

diff --git a/contrib/populate-extfs.sh b/contrib/populate-extfs.sh
old mode 100755
new mode 100644
index b1d3d1f..a84ec74
--- a/contrib/populate-extfs.sh
+++ b/contrib/populate-extfs.sh
@@ -44,7 +44,7 @@ fi
fi

# Only stat once since stat is a time consuming command
- STAT=$(stat -c "TYPE=\"%F\";DEVNO=\"0x%t 0x%T\";MODE=\"%f\";U=\"%u\";G=\"%g\"" $FILE)
+ STAT=$(stat -c "TYPE=\"%F\";DEVNO=\"0x%t 0x%T\";MODE=\"%f\";U=\"%u\";G=\"%g\"" "$FILE")
eval $STAT

case $TYPE in
@@ -52,20 +52,20 @@ fi
echo "mkdir $TGT"
;;
"regular file" | "regular empty file")
- echo "write $FILE $TGT"
+ echo "write \"$FILE\" \"$TGT\""
;;
"symbolic link")
- LINK_TGT=$(readlink $FILE)
- echo "symlink $TGT $LINK_TGT"
+ LINK_TGT=$(readlink "$FILE")
+ echo "symlink \"$TGT\" \"$LINK_TGT\""
;;
"block special file")
- echo "mknod $TGT b $DEVNO"
+ echo "mknod \"$TGT\" b $DEVNO"
;;
"character special file")
- echo "mknod $TGT c $DEVNO"
+ echo "mknod \"$TGT\" c $DEVNO"
;;
"fifo")
- echo "mknod $TGT p"
+ echo "mknod \"$TGT\" p"
;;
*)
echo "Unknown/unhandled file type '$TYPE' file: $FILE" 1>&2
@@ -73,11 +73,11 @@ fi
esac

# Set the file mode
- echo "sif $TGT mode 0x$MODE"
+ echo "sif \"$TGT\" mode 0x$MODE"

# Set uid and gid
- echo "sif $TGT uid $U"
- echo "sif $TGT gid $G"
+ echo "sif \"$TGT\" uid $U"
+ echo "sif \"$TGT\" gid $G"
done

# Handle the hard links.
--
1.7.10.4

2014-02-28 03:02:57

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 1/2] inode.c: only update the icache for ext2_inode

On Fri, Jan 03, 2014 at 04:15:40AM -0500, Robert Yang wrote:
> We read the cache when:
>
> bufsize == sizeof(struct ext2_inode)
>
> so we should update the cache in the same rule, otherwise there would be
> errors, for example:
>
> cache[0]: cached ino 14 when bufsize = 128 by ext2fs_write_inode_full()
> cache[1]: cached ino 14 when bufsize = 156 by ext2fs_read_inode_full()
>
> Then update the cache:
> cache[0]: cached ino 15 when bufsize = 156 by ext2fs_read_inode_full()
>
> And the ino 14 would hit the cache[1] when bufsize = 128 (but it was
> cached by bufsize = 156), so there would be errors.

So I've been looking at this code, and it looks like what we have is
correct. The inode cache is a writethrough cache of the first 128
bytes of the inode, which means that we always update the on-disk
image as well as updating the first 128 bytes of the inode.

Yoru change would actually add a bug, because if the inode 14 was in
the cache, and then we tried to write a large inode, skipping the
inode cache update would mean that a subsequent read of a original
inode size would result in inode cache not being updated, and there
would thus be stale data in the cache.

Here's a test program which proves that what we have right now is
working. If you think there's a problem, could you perhaps try
adjusting this program to show what you think is the bug?

- Ted

/*
* Test read/write inode functions
*/

#include <string.h>
#include <fcntl.h>
#include <ctype.h>
#include <termios.h>
#include <time.h>
#include <getopt.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <sys/ioctl.h>
#include <sys/types.h>

#include <et/com_err.h>
#include <ext2fs/ext2fs.h>

extern int isatty(int);

static const char * program_name = "tst_rwinode";
static const char * device_name = NULL;
static int debug = 0;

static void usage(void)
{
fprintf(stderr,
"Usage: %s [-F] [-I inode_buffer_blocks] device\n",
program_name);
exit(1);
}

static void PRS(int argc, char *argv[])
{
int flush = 0;
int c;
#ifdef MTRACE
extern void *mallwatch;
#endif
errcode_t retval;

setbuf(stdout, NULL);
setbuf(stderr, NULL);
add_error_table(&et_ext2_error_table);

if (argc && *argv)
program_name = *argv;
while ((c = getopt (argc, argv, "d")) != EOF)
switch (c) {
case 'd':
debug++;
break;
default:
usage ();
}
device_name = argv[optind];
}

int main (int argc, char *argv[])
{
errcode_t retval = 0;
ext2_filsys fs;
ext2_ino_t ino;
__u32 num_inodes = 0;
struct ext2_inode_large large_inode;
struct ext2_inode inode;
ext2_inode_scan scan;
int err = 0;

PRS(argc, argv);

retval = ext2fs_open(device_name, EXT2_FLAG_RW,
0, 0, unix_io_manager, &fs);
if (retval) {
com_err(program_name, retval, "while trying to open '%s'",
device_name);
exit(1);
}

retval = ext2fs_read_bitmaps(fs);
if (retval) {
com_err(program_name, retval, "while reading bitmaps");
exit(1);
}

retval = ext2fs_new_inode(fs, EXT2_ROOT_INO, 0644,
NULL, &ino);
if (retval) {
com_err(program_name, retval,
"while trying to allocate inode");
exit(1);
}

if (debug)
printf("Using inode %u\n", ino);

memset(&inode, 0, sizeof(inode));
inode.i_size = 123;

retval = ext2fs_write_new_inode(fs, ino, &inode);
if (retval) {
com_err(program_name, retval,
"while trying to write inode %u", ino);
exit(1);
}

memset(&inode, 0, sizeof(inode));

retval = ext2fs_read_inode(fs, ino, &inode);
if (retval) {
com_err(program_name, retval,
"while reading inode %u", ino);
exit(1);
}
printf("inode size: %u\n", inode.i_size);
if (inode.i_size != 123) {
printf("Bad inode size!!\n");
err++;
}

memset(&large_inode, 0, sizeof(large_inode));
retval = ext2fs_read_inode_full(fs, ino,
(struct ext2_inode *) &large_inode,
sizeof(large_inode));
if (retval) {
com_err(program_name, retval,
"while reading large inode %u", ino);
exit(1);
}

printf("large inode size: %u\n", large_inode.i_size);
if (large_inode.i_size != 123) {
printf("Bad inode size!!\n");
err++;
}

large_inode.i_size = 456;
large_inode.i_crtime = 789;

retval = ext2fs_write_inode_full(fs, ino,
(struct ext2_inode *) &large_inode,
sizeof(large_inode));
if (retval) {
com_err(program_name, retval,
"while write large inode %u", ino);
exit(1);
}

memset(&inode, 0, sizeof(inode));
retval = ext2fs_read_inode(fs, ino, &inode);
if (retval) {
com_err(program_name, retval,
"while reading inode %u", ino);
exit(1);
}
printf("inode size: %u\n", inode.i_size);
if (inode.i_size != 456) {
printf("Bad inode size!!\n");
err++;
}

memset(&large_inode, 0, sizeof(large_inode));
retval = ext2fs_read_inode_full(fs, ino,
(struct ext2_inode *) &large_inode,
sizeof(large_inode));
if (retval) {
com_err(program_name, retval,
"while reading large inode %u", ino);
exit(1);
}

printf("large inode size: %u\n", large_inode.i_size);
printf("large inode crtime: %u\n", large_inode.i_crtime);

if (large_inode.i_size != 456) {
printf("Bad inode size!!\n");
err++;
}
if ((EXT2_INODE_SIZE(fs->super) > 128) &&
large_inode.i_crtime != 789) {
printf("Bad crtime!!\n");
err++;
}

retval = ext2fs_close(fs);
if (retval) {
com_err(program_name, retval,
"while trying to close the filesystem");
exit(1);
}

if (err == 0)
printf("Test succeed!\n");

remove_error_table(&et_ext2_error_table);
exit(err);
}