Add a pair of system calls to make extended file stats available [ver #3]
in [Kernel]
|
From: Andreas Dilger on 2 Jul 2010 12:00 On 2010-07-01, at 23:36, Michael Kerrisk wrote: > * Include information from the "inode_info" structure, most notably > i_flags, but perhaps other info as well. This one is actually pretty interesting, though instead of exporting the i_flags directly (the S_* flags), it would be much better to export the FS_*_FL values. The FS_*_FL values (e.g. FS_IMMUTABLE_FL) are already exposed to userspace via FS_IOC_{GET,SET}FLAGS and are stored on disk in ext2/3/4, so are guaranteed never to change. The S_* flags DO in fact change between kernel releases. Cheers, Andreas -- 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: Andreas Dilger on 4 Jul 2010 17:40 On 2010-06-30, at 17:36, David Howells wrote: > Add a pair of system calls to make extended file stats available, including > file creation time, inode version and data version where available through the > underlying filesystem. I think we could spend forever discussing minor details (e.g. if it would be better to put the st_result_mask as the first field, or at least before st_gen), but it looks fairly reasonable as-is. > When the system call is executed, the request_mask bitmask is read from the > parameter block to work out what the user is requesting. If params is NULL, then request_mask will be assumed to be XSTAT_REQUEST__GET_ANYWAY. I think the only reasonable default if params == NULL is to return XSTAT_REQUEST__BASIC_STATS. The value of XSTAT_REQUEST__EXTENDED_STATS may (AFAIK) change in the kernel in the future as the struct xstat gets more fields, unless that is not the intention. The other option would be to rename this mask XSTAT_REQUEST__BASIC_XSTATS to indicate it represents (forever) all of the fields in the original struct xstat. > +int afs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) > { > + > + stat->result_mask |= XSTAT_REQUEST_GEN | XSTAT_REQUEST_DATA_VERSION; > + stat->gen = inode->i_generation; > + stat->data_version = inode->i_version; You didn't update the field names in any of the kernel patches, > @@ -994,6 +994,8 @@ int ecryptfs_getattr(struct vfsmount *mnt, > + lower_stat.query_flags = stat->query_flags; > + lower_stat.request_mask = stat->request_mask | XSTAT_REQUEST_BLOCKS; > rc = vfs_getattr(ecryptfs_dentry_to_lower_mnt(dentry), > ecryptfs_dentry_to_lower(dentry), &lower_stat); Likewise, this doesn't have the newer field names. I also don't understand why this is adding in the XSTAT_REQUEST_BLOCKS mask when that isn't requested by the caller? > int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) > { > + if ((force || stat->request_mask & (XSTAT_REQUEST_MTIME | > + XSTAT_REQUEST_CTIME | > + XSTAT_REQUEST_DATA_VERSION > + )) && > + S_ISREG(inode->i_mode) > + ) { Minor nit - the parenthesis placement here looks decidedly unLinuxy. Normally I think it should look like: if ((force || stat->request_mask & (XSTAT_REQUEST_MTIME | XSTAT_REQUEST_CTIME | XSTAT_REQUEST_DATA_VERSION)) && S_ISREG(inode->i_mode)) { > + if (force || stat->request_mask & (XSTAT_REQUEST__BASIC_STATS | > + XSTAT_REQUEST_DATA_VERSION) > + ) { Likewise, I think the right style here is: if (force || stat->request_mask & (XSTAT_REQUEST__BASIC_STATS | XSTAT_REQUEST_DATA_VERSION)) { Cheers, Andreas -- 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 5 Jul 2010 10:20 Andreas Dilger <adilger(a)dilger.ca> wrote: > > Add a pair of system calls to make extended file stats available, > > including file creation time, inode version and data version where > > available through the underlying filesystem. > > I think we could spend forever discussing minor details (e.g. if it would be > better to put the st_result_mask as the first field, or at least before > st_gen), but it looks fairly reasonable as-is. I just arranged things such that all the fields of the same type were together for packing purposes, but st_result_mask could easily be brought to the front without upsetting structure packing. > > When the system call is executed, the request_mask bitmask is read from > > the parameter block to work out what the user is requesting. If params is > > NULL, then request_mask will be assumed to be XSTAT_REQUEST__GET_ANYWAY. > > I think the only reasonable default if params == NULL is to return > XSTAT_REQUEST__BASIC_STATS. The value of XSTAT_REQUEST__EXTENDED_STATS may > (AFAIK) change in the kernel in the future as the struct xstat gets more > fields, unless that is not the intention. The other option would be to > rename this mask XSTAT_REQUEST__BASIC_XSTATS to indicate it represents > (forever) all of the fields in the original struct xstat. This is reasonable. I've made that change. I've also fixed the commit message to not mention XSTAT_REQUEST__GET_ANYWAY anymore. > > +int afs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) > > { > > + > > + stat->result_mask |= XSTAT_REQUEST_GEN | XSTAT_REQUEST_DATA_VERSION; > > + stat->gen = inode->i_generation; > > + stat->data_version = inode->i_version; > > You didn't update the field names in any of the kernel patches, What do you mean? That's the kstat struct, not the xstat struct being set in your excerpt. Certainly 'inode_version' has become 'gen' in it and 'result_mask' now appears. > > @@ -994,6 +994,8 @@ int ecryptfs_getattr(struct vfsmount *mnt, > > + lower_stat.query_flags = stat->query_flags; > > + lower_stat.request_mask = stat->request_mask | XSTAT_REQUEST_BLOCKS; > > rc = vfs_getattr(ecryptfs_dentry_to_lower_mnt(dentry), > > ecryptfs_dentry_to_lower(dentry), &lower_stat); > > > Likewise, this doesn't have the newer field names. Doesn't it? Where? I'm unsure as to whether I should copy across the btime, data_version and gen fields here. > I also don't understand why this is adding in the XSTAT_REQUEST_BLOCKS mask > when that isn't requested by the caller? Because it is wanted by ecryptfs_getattr: stat->blocks = lower_stat.blocks; though possibly this should be contingent on XSTAT_REQUEST_BLOCKS being set in stat->request_mask. > > int nfs_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat) > > { > > + if ((force || stat->request_mask & (XSTAT_REQUEST_MTIME | > > + XSTAT_REQUEST_CTIME | > > + XSTAT_REQUEST_DATA_VERSION > > + )) && > > + S_ISREG(inode->i_mode) > > + ) { > > Minor nit - the parenthesis placement here looks decidedly unLinuxy. Normally I think it should look like: It makes it easier to see that the S_ISREG is part of the if-condition and not part of the body. 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: David Howells on 5 Jul 2010 11:10 Michael Kerrisk <mtk.manpages(a)gmail.com> wrote: > * Include information from the "inode_info" structure, most notably > i_flags, but perhaps other info as well. This thought has occurred to me, but are the contents of i_flags identical for all filesystems? Certainly, i_flags doesn't seem to match the FS_IOC_GETFLAGS mask. For example: #define FS_SECRM_FL 0x00000001 vs: #define S_SYNC 1 /* Writes are synced at once */ I've also been asked to provide st_flags as for BSD, which aren't compatible either:-/. Some questions: (1) Does it make sense to rearrange the S_xxxx flags to match the numbers for FS_xxxx_FL? (2) Does it make sense to do the BSD st_flags compatibility in userspace? (3) Can we add a couple more flags to make Samba's life easier? Say an archived bit and a hidden bit? (4) Do I actually need to provide a mask saying what st_flags are applicable to the file you've just queried? (5) How often are these flags required? E.g. does it make more sense to keep them as an additional result, or does it make sense to stick them in the kstat and xstat structs, knowing that these are allocated on the kernel stack maybe as three times if an ecryptfs file? > * Return a bit mask indicating the presence of additional information > associated with the i-node. Here, I am thinking of flags that indicate > that the file has any of the following: capabilities, an ACL, and > extended attributes (obviously a superset of the previous). I could > imagine some apps that, having got the xstat info, would be interested > to obtain some of this other info. > > Obviously, the above only make sense if the overhead of providing the > extra information is low. That might make sense as an 'additional result'. These things may have to be probed for on disk or on a server, so you might not want to return them by default, and you may want to indicate what the filesystem can support vs what the file actually has: u64 st_fs_additional_info; /* what the filesystem supports */ u64 st_file_additional_info; /* what the file actually has */ #define XST_ADDINFO_CAPABILITY_MASK #define XST_ADDINFO_ACL #define XST_ADDINFO_XATTRS #define XST_ADDINFO_SECLABEL 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: Andreas Dilger on 7 Jul 2010 11:00 On 2010-07-05, at 08:59, David Howells wrote: > Michael Kerrisk <mtk.manpages(a)gmail.com> wrote: > >> * Include information from the "inode_info" structure, most notably >> i_flags, but perhaps other info as well. > > This thought has occurred to me, but are the contents of i_flags identical for > all filesystems? Certainly, i_flags doesn't seem to match the FS_IOC_GETFLAGS > mask. For example: > > #define FS_SECRM_FL 0x00000001 > > vs: > > #define S_SYNC 1 /* Writes are synced at once */ > > (1) Does it make sense to rearrange the S_xxxx flags to match the numbers for > FS_xxxx_FL? I saw your patch to that effect. I'm of mixed feelings about this, since the S_* flags have traditionally been changed on an ad-hoc basis and I don't necessarily want to let this leak into the on-disk format of these flags for ext*. One way to ensure that this holds true is to have a compile-time assertion that the respective S_* flags match FS_*_FL and EXT_*_FL. We use a macro in Lustre for compile-time assertions that depends on the detection of duplicate values in a switch() statement: /** Compile-time assertion. * Check an invariant described by a constant expression at compile time by * forcing a compiler error if it does not hold. @cond must be a constant * expression as defined by the ISO C Standard: * * 6.8.4.2 The switch statement * .... * [#3] The expression of each case label shall be an integer * constant expression and no two of the case constant * expressions in the same switch statement shall have the same * value after conversion... * */ #define CLASSERT(cond) do {switch(42) {case (cond): case 0: break;}} while (0) > (2) Does it make sense to do the BSD st_flags compatibility in userspace? > > (3) Can we add a couple more flags to make Samba's life easier? Say an > archived bit and a hidden bit? I wouldn't object to that. The BSD flags indicate that the hidden bit should only affect GUI display managers, not "ls". > (4) Do I actually need to provide a mask saying what st_flags are applicable > to the file you've just queried? Hmm, good question. > (5) How often are these flags required? E.g. does it make more sense to keep > them as an additional result, or does it make sense to stick them in the > kstat and xstat structs, knowing that these are allocated on the kernel > stack maybe as three times if an ecryptfs file? If they aren't requested by userspace, the cost is mostly irrelevant. I think on OSX these flags are returned for every "ls" call, to mark the inodes with xattrs every time. Cheers, Andreas -- 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/
|
Next
|
Last
Pages: 1 2 Prev: [GIT PULL] AMD64 EDAC fixlet for 2.6.35-rc4 Next: iwl3945: HARDWARE GONE?? |