| Message ID | 1388740541-28123-2-git-send-email-liezhi.yang@windriver.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
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); } -- 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
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 {
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(-)