* [PATCH 0/2] e2fsprogs: two fixes @ 2014-01-03 9:15 Robert Yang 2014-01-03 9:15 ` [PATCH 1/2] inode.c: only update the icache for ext2_inode Robert Yang 2014-01-03 9:15 ` [PATCH 2/2] populate-extfs.sh: espace the space in the filename Robert Yang 0 siblings, 2 replies; 4+ messages in thread From: Robert Yang @ 2014-01-03 9:15 UTC (permalink / raw) To: tytso, sgh; +Cc: linux-ext4 * 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] inode.c: only update the icache for ext2_inode 2014-01-03 9:15 [PATCH 0/2] e2fsprogs: two fixes Robert Yang @ 2014-01-03 9:15 ` Robert Yang 2014-02-28 2:09 ` Theodore Ts'o 2014-01-03 9:15 ` [PATCH 2/2] populate-extfs.sh: espace the space in the filename Robert Yang 1 sibling, 1 reply; 4+ messages in thread From: Robert Yang @ 2014-01-03 9:15 UTC (permalink / raw) To: tytso, sgh; +Cc: linux-ext4 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 <liezhi.yang@windriver.com> --- 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] inode.c: only update the icache for ext2_inode 2014-01-03 9:15 ` [PATCH 1/2] inode.c: only update the icache for ext2_inode Robert Yang @ 2014-02-28 2:09 ` Theodore Ts'o 0 siblings, 0 replies; 4+ messages in thread From: Theodore Ts'o @ 2014-02-28 2:09 UTC (permalink / raw) To: Robert Yang; +Cc: sgh, linux-ext4 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); } ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] populate-extfs.sh: espace the space in the filename 2014-01-03 9:15 [PATCH 0/2] e2fsprogs: two fixes Robert Yang 2014-01-03 9:15 ` [PATCH 1/2] inode.c: only update the icache for ext2_inode Robert Yang @ 2014-01-03 9:15 ` Robert Yang 1 sibling, 0 replies; 4+ messages in thread From: Robert Yang @ 2014-01-03 9:15 UTC (permalink / raw) To: tytso, sgh; +Cc: linux-ext4 From: Søren Holm <sgh@sgh.dk> The filename which contains space would not get into the final ext2/3/4 filsystem without this patch Signed-off-by: Søren Holm <sgh@sgh.dk> Signed-off-by: Robert Yang <liezhi.yang@windriver.com> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-28 3:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-01-03 9:15 [PATCH 0/2] e2fsprogs: two fixes Robert Yang 2014-01-03 9:15 ` [PATCH 1/2] inode.c: only update the icache for ext2_inode Robert Yang 2014-02-28 2:09 ` Theodore Ts'o 2014-01-03 9:15 ` [PATCH 2/2] populate-extfs.sh: espace the space in the filename Robert Yang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).