[PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Li Zefan]

Add EXT4_IOC_GETCRTIME ioctl, and then we can use it in
e2fsprogs.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 fs/ext4/ext4.h  |    1 +
 fs/ext4/ioctl.c |    3 +++
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 303e41c..a487f2f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -295,6 +295,7 @@ struct ext4_new_group_data {
 #define EXT4_IOC_GROUP_ADD		_IOW('f', 8,struct ext4_new_group_input)
 #define	EXT4_IOC_GETVERSION_OLD		FS_IOC_GETVERSION
 #define	EXT4_IOC_SETVERSION_OLD		FS_IOC_SETVERSION
+#define	EXT4_IOC_GETCRTIME		_IOR('f', 9, struct timespec)
 #ifdef CONFIG_JBD2_DEBUG
 #define EXT4_IOC_WAIT_FOR_READONLY	_IOR('f', 99, long)
 #endif
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7a6c2f1..6204b5d 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -269,6 +269,9 @@ setversion_out:
 	case EXT4_IOC_MIGRATE:
 		return ext4_ext_migrate(inode, filp, cmd, arg);
 
+	case EXT4_IOC_GETCRTIME:
+		return put_user(ei->i_crtime, (struct timespec __user *)arg);
+
 	default:
 		return -ENOTTY;
 	}
-- 
1.5.4.rc3

Re: [PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Theodore Tso]

On Wed, Aug 06, 2008 at 05:09:07PM +0800, Li Zefan wrote:
> +	case EXT4_IOC_GETCRTIME:
> +		return put_user(ei->i_crtime, (struct timespec __user *)arg);
> +

I'm worried about writing a struct timespec directly to user space,
because the kernel's idea of what is struct timespec might not be the
same as the userspace's understanding of struct timespec ---
specifically, because of the question of the width of time_t might be
different in the kernel and in userspace on different architectures.

I think we would be better off explicitly defining a structure, or
just returning the seconds and nanoseconds in explicit primitive
types.

Regards,

	     	 	       		 - Ted

Re: [PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Li Zefan]

(sorry for the so dealyed reply, I was offline last week.)

Theodore Tso wrote:
> On Wed, Aug 06, 2008 at 05:09:07PM +0800, Li Zefan wrote:
>> +	case EXT4_IOC_GETCRTIME:
>> +		return put_user(ei->i_crtime, (struct timespec __user *)arg);
>> +
> 
> I'm worried about writing a struct timespec directly to user space,
> because the kernel's idea of what is struct timespec might not be the
> same as the userspace's understanding of struct timespec ---

We have system call nanosleep(), which copies a struct timespec directly
from user space.

> specifically, because of the question of the width of time_t might be
> different in the kernel and in userspace on different architectures.
> 

But timeval.tv_sec is also of type time_t. Also sys_time() writes a time_t
directry to user space.

I should not use put_user() though...

+		return copy_to_user((struct timespec __user *)arg,
+ 			&ei->i_crtime, sizeof(ei->i_crtime));

> I think we would be better off explicitly defining a structure, or
> just returning the seconds and nanoseconds in explicit primitive
> types.
> 

Re: [PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Theodore Tso]

On Mon, Aug 18, 2008 at 10:08:11AM +0800, Li Zefan wrote:
> > 
> > I'm worried about writing a struct timespec directly to user space,
> > because the kernel's idea of what is struct timespec might not be the
> > same as the userspace's understanding of struct timespec ---
> 
> We have system call nanosleep(), which copies a struct timespec directly
> from user space.

The difference is that for system calls, we have a glue layer (glibc)
whose duties include translating between the kernel data structures
and the userpsace data structures --- and for various architectures
there are ***no*** guarantees that the interfaces shipped by glibc in
/usr/include match up with the data structures used by the kernel in
/usr/src/linux/include/linux.

When you use an ioctl, you bypass the glibc translation layer, so life
can get iffy here.  And given that struct timespec contains time_t,
which *can* differ from architecture to architecure in in terms of
being either 32 bits or 64 bits, and what is in the kernel might be
different from what is in the user space /usr/include, I get doubly
nervous.

> > I think we would be better off explicitly defining a structure, or
> > just returning the seconds and nanoseconds in explicit primitive
> > types.

That's the quick and dirty fast answer, yes.  The long-term (but one
which involves much more work) is to define a new struct
kernel<->glibc stat interface (we already have 5 or so :-) to extend
it include st_crtime, and then try to get glibc to use the magic of
ELF symbol versioning so there is a new struct stat as defined in
/usr/include, and a new stat(2) call defined in glibc, which returns
the new struct stat which include st_crtime.  This also means we have
to define proper semantics for what happens if a filesystem doesn't
support st_crtime.

						- Ted

Re: [PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Andreas Dilger]

On Aug 17, 2008  22:36 -0400, Theodore Ts'o wrote:
> That's the quick and dirty fast answer, yes.  The long-term (but one
> which involves much more work) is to define a new struct
> kernel<->glibc stat interface (we already have 5 or so :-) to extend
> it include st_crtime, and then try to get glibc to use the magic of
> ELF symbol versioning so there is a new struct stat as defined in
> /usr/include, and a new stat(2) call defined in glibc, which returns
> the new struct stat which include st_crtime.  This also means we have
> to define proper semantics for what happens if a filesystem doesn't
> support st_crtime.

In the meantime, I'd think it preferable to use a virtual-xattr to get
this information from the kernel instead of an ioctl.  That way a shell
script can extract it with getfattr, and it is no harder to do from a
C program using getxattr().

I'd propose something like "system.creation_time" for the xattr name,
but I'll leave it up to people who care to propose better names.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Re: [PATCH 1/3] ext4: add EXT4_IOC_GETCRTIME ioctl

[Li Zefan]

Theodore Tso wrote:
> On Mon, Aug 18, 2008 at 10:08:11AM +0800, Li Zefan wrote:
>>> I'm worried about writing a struct timespec directly to user space,
>>> because the kernel's idea of what is struct timespec might not be the
>>> same as the userspace's understanding of struct timespec ---
>> We have system call nanosleep(), which copies a struct timespec directly
>> from user space.
> 
> The difference is that for system calls, we have a glue layer (glibc)
> whose duties include translating between the kernel data structures
> and the userpsace data structures --- and for various architectures
> there are ***no*** guarantees that the interfaces shipped by glibc in
> /usr/include match up with the data structures used by the kernel in
> /usr/src/linux/include/linux.
> 
> When you use an ioctl, you bypass the glibc translation layer, so life
> can get iffy here.  And given that struct timespec contains time_t,
> which *can* differ from architecture to architecure in in terms of
> being either 32 bits or 64 bits, and what is in the kernel might be
> different from what is in the user space /usr/include, I get doubly
> nervous.
> 

I got the point, thanks. :)

>>> I think we would be better off explicitly defining a structure, or
>>> just returning the seconds and nanoseconds in explicit primitive
>>> types.
> 
> That's the quick and dirty fast answer, yes.  The long-term (but one
> which involves much more work) is to define a new struct
> kernel<->glibc stat interface (we already have 5 or so :-) to extend
> it include st_crtime, and then try to get glibc to use the magic of
> ELF symbol versioning so there is a new struct stat as defined in
> /usr/include, and a new stat(2) call defined in glibc, which returns
> the new struct stat which include st_crtime.  This also means we have
> to define proper semantics for what happens if a filesystem doesn't
> support st_crtime.
> 

Yes, my first thought was if stat can report crtime.

So, for now I think we can use timespec_to_ns() to convert the time to
a s64 value and return it to the userspace.