On Jul 10, 2007 16:30 -0700, Andrew Morton wrote:
> > +#define EXT4_FITS_IN_INODE(ext4_inode, einode, field) \
> > + ((offsetof(typeof(*ext4_inode), field) + \
> > + sizeof((ext4_inode)->field)) \
> > + <= (EXT4_GOOD_OLD_INODE_SIZE + \
> > + (einode)->i_extra_isize)) \
>
> Please add explanatory commentary to EXT4_FITS_IN_INODE(): tell readers
> under what circumstances something will not fit in an inode and what the
> consequences of this are.
/* Extended fields will fit into an inode if the filesystem was formatted
* with large inodes (-I 256 or larger) and there are not currently EAs
* consuming all of the available space. For new inodes we always reserve
* enough space for the kernel's known extended fields, but for inodes
* created with an old kernel this might not have been the case. None of
* the extended inode fields is critical for correct filesystem operation.
*/
> > +#define EXT4_INODE_GET_XTIME(xtime, inode, raw_inode) \
> > +do { \
> > + (inode)->xtime.tv_sec = le32_to_cpu((raw_inode)->xtime); \
> > + if (EXT4_FITS_IN_INODE(raw_inode, EXT4_I(inode), xtime ## _extra)) \
> > + ext4_decode_extra_time(&(inode)->xtime, \
> > + raw_inode->xtime ## _extra); \
> > +} while (0)
>
> Ugly. I expect these could be implemented as plain old C functions.
> Caller could pass in the address of the ext4_inode field which the function
> is to operate upon.
We thought about that also, but then the caller needs to do all of the
pointer gymnastics themselves like:
ext4_inode_get_xtime(&inode->i_ctime, &inode->i_ctime_extra,
&raw_inode->i_ctime, &raw_inode->i_ctime_extra)
instead of the current:
EXT4_INODE_GET_XTIME(ctime, inode, raw_inode);
IMHO it is preferrable to make the multiple callsites more readable than
the macros.
> > #if defined(__KERNEL__) || defined(__linux__)
>
> (What's the __linux__ for?)
>
> > #define i_reserved1 osd1.linux1.l_i_reserved1
> > #define i_frag osd2.linux2.l_i_frag
This is actually unrelated to the current patch, just part of the context.
AFAIK, this is historical, so that the kernel and e2fsprogs can use the
same ext2_fs.h header. I don't think it is really needed, but such cleanup
shouldn't be a part of this patch either.
> > +static inline struct timespec ext4_current_time(struct inode *inode)
> > +{
> > + return (inode->i_sb->s_time_gran < NSEC_PER_SEC) ?
> > + current_fs_time(inode->i_sb) : CURRENT_TIME_SEC;
> > +}
>
> Now, I've forgotten how this works. Remind me, please. Can ext4
> filesystems ever have one-second timestamp granularity? If so, how does
> one cause that to come about?
Yes, this is possible if an ext2/3/4 filesystem is formatted with 128-byte
inodes (which is the default for all but ext4) and this fs is mounted as
ext4dev. The inodes can never hold the extra time information (FITS_IN_INODE
check above) so the superblock limits the timestamp resolution to 1s in that
case.
> > @@ -153,6 +153,7 @@
> >
> > unsigned long i_ext_generation;
> > struct ext4_ext_cache i_cached_extent;
> > + struct timespec i_crtime;
> > };
>
> It is unobvious what this field does. Please prefer to add commentary to
> _all_ struct fields - it really helps.
It is the inode creation time. This is useful for debug/forensic purposes,
and at some point there will be an API so that Samba can use it also.
> > #endif /* _LINUX_EXT4_FS_I */
> > Index: linux-2.6.22-rc4/include/linux/ext4_fs_sb.h
> > ===================================================================
> > --- linux-2.6.22-rc4.orig/include/linux/ext4_fs_sb.h 2007-06-11 17:28:15.000000000 -0700
> > +++ linux-2.6.22-rc4/include/linux/ext4_fs_sb.h 2007-06-11 17:39:05.000000000 -0700
> > @@ -79,6 +79,7 @@
> > char *s_qf_names[MAXQUOTAS]; /* Names of quota files with journalled quota */
> > int s_jquota_fmt; /* Format of quota to use */
> > #endif
> > + unsigned int s_want_extra_isize; /* New inodes should reserve # bytes */
>
> OK, I can kind-of see how this is working, but some overall description of
> how the inode sizing design operates would be helpful. It would certainly
> make reviewing of this proposed change more fruitful. Perhaps that new
> comment over EXT4_FITS_IN_INODE() would be a suitable place.
Hmm, I'm sure there were emails on the topic, but they aren't attached to
the patch. s_want_extra_isize is just an override for sizeof(ext4_inode)
in case the sysadmin wants to reserve more fields in new inodes. There is
also s_min_extra_isize which is what the kernel and e2fsck guarantee that
will be available in all in-use inodes, if RO_COMPAT_EXTRA_ISIZE is set
(ro-compat so that older kernels can't create inodes with a smaller
extra_isize). That feature is only enabled if requested by the sysadmin.
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
