Rearrange i_flags to be consistent with FS_IOC_GETFLAGS
in [Kernel]
|
Prev: [PATCH] Rearrange i_flags to be consistent with FS_IOC_GETFLAGS
Next: soundcore_open: Reduce the area BKL coverage
From: Eric Sandeen on 5 Jul 2010 12:00 David Howells wrote: > Rearrange the constituent bits of i_flags to be consistent with the flag values > returned by the FS_IOC_GETFLAGS ioctl where flags with common meanings occur. > Otherwise pack the bits of i_flags as low as possible so that RISC CPUs can > use small instructions. > > This allows those filesystems that use i_flags (Ext2/3/4) to simplify their > get/set flags routines. > > Signed-off-by: David Howells <dhowells(a)redhat.com> Seems like a good idea to me, although: > diff --git a/fs/ext2/inode.c b/fs/ext2/inode.c > index 3675088..da5fd2d 100644 > --- a/fs/ext2/inode.c > +++ b/fs/ext2/inode.c > @@ -1259,38 +1259,17 @@ Egdp: > > void ext2_set_inode_flags(struct inode *inode) > { > - unsigned int flags = EXT2_I(inode)->i_flags; > - > - inode->i_flags &= ~(S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC); > - if (flags & EXT2_SYNC_FL) > - inode->i_flags |= S_SYNC; > - if (flags & EXT2_APPEND_FL) > - inode->i_flags |= S_APPEND; > - if (flags & EXT2_IMMUTABLE_FL) > - inode->i_flags |= S_IMMUTABLE; > - if (flags & EXT2_NOATIME_FL) > - inode->i_flags |= S_NOATIME; > - if (flags & EXT2_DIRSYNC_FL) > - inode->i_flags |= S_DIRSYNC; Maybe a stern comment should be here about the relationship between this and the flag definitions... > + unsigned int flags = EXT2_I(inode)->i_flags & S_FS_IOC_FLAGS; > + > + inode->i_flags = (inode->i_flags & ~S_FS_IOC_FLAGS) | flags; > } > .... > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 48616db..5808b9b 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h .... > +/* > + * Inode flags - they have no relation to superblock flags now > + * > + * Where applicable, flags that are also in FS_IOC_GETFLAGS have the same bit > + * position > + */ .... a stern comment here about not re-ordering since other fs/*/* code depends strongly on this order? "where applicable" sounds like a wish, but now these can't be changed, right? If somebody rearranges these bad things will happen. -Eric > +#define S_DEAD 0x00000001 /* removed, but still open directory */ > +#define S_NOQUOTA 0x00000002 /* Inode is not counted to quota */ > +#define S_NOCMTIME 0x00000004 /* Do not update file c/mtime */ > +#define S_SYNC 0x00000008 /* [FS_SYNC_FL] Writes are synced at once */ > +#define S_IMMUTABLE 0x00000010 /* [FS_IMMUTABLE_FL] Immutable file */ > +#define S_APPEND 0x00000020 /* [FS_APPEND_FL] Append-only file */ > +#define S_SWAPFILE 0x00000040 /* Do not truncate: swapon got its bmaps */ > +#define S_NOATIME 0x00000080 /* [FS_NOATIME_FL] Do not update access times */ > +#define S_PRIVATE 0x00000100 /* Inode is fs-internal */ > +#define S_DIRSYNC 0x00010000 /* [FS_DIRSYNC_FL] Directory mods are synchronous */ > + > +#define S_FS_IOC_FLAGS 0x000100B8 /* the S_xxx flags that are also FS_xxx_FL flags */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: tytso on 5 Jul 2010 13:30 On Mon, Jul 05, 2010 at 10:54:07AM -0500, Eric Sandeen wrote: > > ... a stern comment here about not re-ordering since other fs/*/* > code depends strongly on this order? A stern warning is needed there, but it's also needed a few lines further down in the inode flags section where the FS_*_FL flags are defined. These were originally ext2-specific inode flags, and it's become generalized to a fs-independent set of bit fields, but what's nasty/important to remember is that these flags are also used as on-disk flags for ext2/3/4, and so extreme care is needed before new flags are for FS_IOC_GETFLAGS/FS_IOC_SETFLAGS are allocated.... In fact, I'd argue it's much more strongly needed for the FS_*_FL flags. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dave Chinner on 5 Jul 2010 20:20 On Mon, Jul 05, 2010 at 04:43:19PM +0100, David Howells wrote: > Rearrange the constituent bits of i_flags to be consistent with the flag values > returned by the FS_IOC_GETFLAGS ioctl where flags with common meanings occur. > Otherwise pack the bits of i_flags as low as possible so that RISC CPUs can > use small instructions. > > This allows those filesystems that use i_flags (Ext2/3/4) to simplify their > get/set flags routines. This seems like a nasty landmine just waiting for someone to step on. The S_* flags are VFS level flags used by all filesytsems, not just extN, and have a history of changing values and meaning as uses come and go. This code forces the VFS to use certain flag values that *match on-disk values* of ext2/3/4. Further, it opens up the possibility that further down the track someone modifies S_* flag values (say when adding some XFS functionality) and corrupts extN filesystems all over the place. There isn't even compiler guards to catch someone modifying the S_* flags in a way that makes it incompatible with the extN on-disk definitions. Cheers, Dave. -- Dave Chinner david(a)fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 6 Jul 2010 09:50 Dave Chinner <david(a)fromorbit.com> wrote: > Further, it opens up the possibility that further down the track > someone modifies S_* flag values (say when adding some XFS > functionality) and corrupts extN filesystems all over the place. > There isn't even compiler guards to catch someone modifying the S_* > flags in a way that makes it incompatible with the extN on-disk > definitions. That occurred to me after I sent the patch. I can add some preprocessor guards for this. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Dave Chinner on 6 Jul 2010 19:10
On Tue, Jul 06, 2010 at 02:40:23PM +0100, David Howells wrote: > Dave Chinner <david(a)fromorbit.com> wrote: > > > Further, it opens up the possibility that further down the track > > someone modifies S_* flag values (say when adding some XFS > > functionality) and corrupts extN filesystems all over the place. > > There isn't even compiler guards to catch someone modifying the S_* > > flags in a way that makes it incompatible with the extN on-disk > > definitions. > > That occurred to me after I sent the patch. I can add some preprocessor guards > for this. I'd prefer generic flags are not dependent on fixed values from a specific filesystem several layers down the storage stack. Also, if the problem you are trying to solve is overhead of calculating the flags for stat() on RISC architectures, then I'd argue that XFS is just as important target for such an optimisation because it is widely used in small ARM and MIPS based NAS appliances.... Cheers, Dave. -- Dave Chinner david(a)fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |