btrfs_inode_item's otime?

btrfs_inode_item's otime?

[Lennart Poettering]

Heya!

btrfs' btrfs_inode_item structure contains a field for the birth time
of a file, .otime. This field could be quite useful, and I'd like to
make use of it. I can query it with the BTRFS_IOC_TREE_SEARCH ioctl
from userspace, alas it appears that the entry is never actually
initialized to anything other than 0?

Is this on purpose, or simply an oversight? It should be easy to
initialize it to the mtime when the inode is first created...

I am aware of the discussions about introducing the birth time as
something queriable with a future xstat() call. Even if that
high-level API doesn't exist yet, and even if it might be messy to use
BTRFS_IOC_TREE_SEARCH to query the otime currently, I think it would
be good to properly initialize the field, so that pre-existing file
systems would report useful data when xstat() is added one day...

(Of course, even without xstat(), I think it would be good to have an
unprivileged ioctl to query the otime in btrfs... the TREE_SEARCH
ioctl after all requires privileges...)

Lennart

-- 
Lennart Poettering, Red Hat

Re: btrfs_inode_item's otime?

[Chris Samuel]

On Mon, 5 Jan 2015 06:21:52 PM Lennart Poettering wrote:

> Is this on purpose, or simply an oversight?


The only hint I can see that it's deliberate is the comment in fs/btrfs/send.c 
that says:

/* TODO Add otime support when the otime patches get into upstream */

However...

> It should be easy to initialize it to the mtime when the inode is
> first created...

This I agree with, well worth doing anyway.

I'll see if I can knock up a patch.

All the best,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

Re: btrfs_inode_item's otime?

[Chris Samuel]

On Tue, 6 Jan 2015 10:47:00 PM Chris Samuel wrote:

> On Mon, 5 Jan 2015 06:21:52 PM Lennart Poettering wrote:
> 
> > It should be easy to initialize it to the mtime when the inode is
> > first created...
> 
> This I agree with, well worth doing anyway.
> 
> I'll see if I can knock up a patch.

Sadly it appears that the btrfs code sets mtime/ctime/atimeat inode creation  
via the normal filesystem inode structure, not through it's own, and as that 
doesn't include otime I'm afraid it's out of my league.  Worth a shot though!

All the best,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

Re: btrfs_inode_item's otime?

[David Sterba]

On Tue, Jan 06, 2015 at 11:43:22PM +1100, Chris Samuel wrote:
> On Tue, 6 Jan 2015 10:47:00 PM Chris Samuel wrote:
> 
> > On Mon, 5 Jan 2015 06:21:52 PM Lennart Poettering wrote:
> > 
> > > It should be easy to initialize it to the mtime when the inode is
> > > first created...
> > 
> > This I agree with, well worth doing anyway.
> > 
> > I'll see if I can knock up a patch.
> 
> Sadly it appears that the btrfs code sets mtime/ctime/atimeat inode creation  
> via the normal filesystem inode structure, not through it's own, and as that 
> doesn't include otime I'm afraid it's out of my league.  Worth a shot though!

Set the otime in btrfs_new_inode after the call to fill_inode_item.

Re: btrfs_inode_item's otime?

[Chris Samuel]

Hi Dave,

Sorry for the delay, took a while to poke around the code to figure
out possible ways it would get done (and what the right structure was).

On Tue, 6 Jan 2015 07:41:00 PM David Sterba wrote:

> Set the otime in btrfs_new_inode after the call to fill_inode_item.

Hmm, so something like this then?

Unfortunately I won't have a test system to try this on for a little while.


diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e687bb0..60bcc72 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5835,6 +5835,11 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
                             sizeof(*inode_item));
        fill_inode_item(trans, path->nodes[0], inode_item, inode);
 
+       /*
+        * Set the creation time on the inode.
+        */
+       btrfs_set_stack_timespec_sec( &inode.otime, cur_time.tv_sec );
+
        if (name) {
                ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
                                     struct btrfs_inode_ref);


-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

Re: btrfs_inode_item's otime?

[David Sterba]

On Thu, Jan 08, 2015 at 09:52:30PM +1100, Chris Samuel wrote:
> Sorry for the delay, took a while to poke around the code to figure
> out possible ways it would get done (and what the right structure was).

No delay noticed :)

> > Set the otime in btrfs_new_inode after the call to fill_inode_item.
> Unfortunately I won't have a test system to try this on for a little while.
> 
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index e687bb0..60bcc72 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -5835,6 +5835,11 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
>                              sizeof(*inode_item));
>         fill_inode_item(trans, path->nodes[0], inode_item, inode);
>  
> +       /*
> +        * Set the creation time on the inode.
> +        */
> +       btrfs_set_stack_timespec_sec( &inode.otime, cur_time.tv_sec );

Drop the spaces after/before parens and also set usec the same way.
There's no such thing as 'current_time', only CURRENT_TIME but that
cannot be used directly as a structure.

Given that the mtime is set a few lines above, copy the tv_sec and
tv_usec from there.

> +
>         if (name) {
>                 ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
>                                      struct btrfs_inode_ref);
> 

Re: btrfs_inode_item's otime?

[David Sterba]

On Fri, Jan 09, 2015 at 05:11:42PM +0100, David Sterba wrote:
> > --- a/fs/btrfs/inode.c
> > +++ b/fs/btrfs/inode.c
> > @@ -5835,6 +5835,11 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
> >                              sizeof(*inode_item));
> >         fill_inode_item(trans, path->nodes[0], inode_item, inode);
> >  
> > +       /*
> > +        * Set the creation time on the inode.
> > +        */
> > +       btrfs_set_stack_timespec_sec( &inode.otime, cur_time.tv_sec );
> 
> Drop the spaces after/before parens and also set usec the same way.
> There's no such thing as 'current_time', only CURRENT_TIME but that
> cannot be used directly as a structure.
> 
> Given that the mtime is set a few lines above, copy the tv_sec and
> tv_usec from there.

chandan pointed out on IRC the other day that he'd sent a patch for that
already

http://www.mail-archive.com/linux-btrfs%40vger.kernel.org/msg17508.html

Though the patch cannot be applied as-is, it's more complete (I've
missed a few places where the otime has to be set).

Chandan, please drop the btrfs_inode_otime helper and resend. Thanks.

Re: btrfs_inode_item's otime?

[Chris Samuel]

On 15/01/15 21:48, David Sterba wrote:

> Chandan, please drop the btrfs_inode_otime helper and resend. Thanks.

Thanks!

Sorry I'd had no further time to look at this, I've been fully committed
with $DAY_JOB and on a number of projects with our local community
observatory (if anyone is in/visiting Melbourne and into astronomy ping
me for details).

All the best,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

Re: btrfs_inode_item's otime?

[David Sterba]

On Mon, Jan 05, 2015 at 06:21:52PM +0100, Lennart Poettering wrote:
> btrfs' btrfs_inode_item structure contains a field for the birth time
> of a file, .otime. This field could be quite useful, and I'd like to
> make use of it. I can query it with the BTRFS_IOC_TREE_SEARCH ioctl
> from userspace, alas it appears that the entry is never actually
> initialized to anything other than 0?
> 
> Is this on purpose, or simply an oversight? It should be easy to
> initialize it to the mtime when the inode is first created...

I'ts probably just lack of implementation due to lack of interface to
userspace, but we should set it.

> I am aware of the discussions about introducing the birth time as
> something queriable with a future xstat() call. Even if that
> high-level API doesn't exist yet, and even if it might be messy to use
> BTRFS_IOC_TREE_SEARCH to query the otime currently, I think it would
> be good to properly initialize the field, so that pre-existing file
> systems would report useful data when xstat() is added one day...

Agreed.

> (Of course, even without xstat(), I think it would be good to have an
> unprivileged ioctl to query the otime in btrfs... the TREE_SEARCH
> ioctl after all requires privileges...)

Adding this interface is a different question. I do not like to add
ioctls that do too specialized things that normally fit into a generic
interface like the xstat example. We could use the object properties
instead (ie. export the otime as an extended attribute), but the work on
that has stalled and it's not ready to just simply add the otime in
advance.

Re: btrfs_inode_item's otime?

[Lennart Poettering]

On Tue, 06.01.15 19:26, David Sterba (dsterba@suse.cz) wrote:

> > (Of course, even without xstat(), I think it would be good to have an
> > unprivileged ioctl to query the otime in btrfs... the TREE_SEARCH
> > ioctl after all requires privileges...)
> 
> Adding this interface is a different question. I do not like to add
> ioctls that do too specialized things that normally fit into a generic
> interface like the xstat example. We could use the object properties
> instead (ie. export the otime as an extended attribute), but the work on
> that has stalled and it's not ready to just simply add the otime in
> advance.

Exposig this as xattr sounds great to me too.

Lennart

-- 
Lennart Poettering, Red Hat

Re: btrfs_inode_item's otime?

[Christoph Hellwig]

On Wed, Jan 07, 2015 at 02:57:35PM +0100, Lennart Poettering wrote:
> Exposig this as xattr sounds great to me too.

NAK - exposing random stat data as xattr only creates problems.

Given that we don't seem to be able to get a new stat format anytime
soon we should add a generic ioctl to expose it, reading it from struct
kstat which all filesystem that support this attribute should fill out.
And there's quite a lot of them.

Re: btrfs_inode_item's otime?

[Martin Steigerwald]

Am Mittwoch, 7. Januar 2015, 06:42:50 schrieb Christoph Hellwig:
> On Wed, Jan 07, 2015 at 02:57:35PM +0100, Lennart Poettering wrote:
> > Exposig this as xattr sounds great to me too.
> 
> NAK - exposing random stat data as xattr only creates problems.
> 
> Given that we don't seem to be able to get a new stat format anytime
> soon we should add a generic ioctl to expose it, reading it from struct
> kstat which all filesystem that support this attribute should fill out.
> And there's quite a lot of them.

What is the issue with the xstat, new stat format stuff? Basically I have the 
feeling that this is in discussion for at least 5 years or so and I always 
wondered where it is stuck. I am not aware of any discussions of it recently, 
but I may have overseen those.

Ext4, I think, already supports birth time, but cannot expose it either. And 
even the stat command already seems to have some support for it, except that 
it doesn´t work yet (this is on BTRFS, but on Ext4 its the same).

merkaba:~> LANG=C stat /bin/ls
  File: '/bin/ls'
  Size: 118280          Blocks: 232        IO Block: 4096   regular file
Device: 14h/20d Inode: 3574391     Links: 1
Access: (0755/-rwxr-xr-x)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2014-10-31 11:29:37.000000000 +0100
Modify: 2014-10-30 03:43:06.000000000 +0100
Change: 2014-10-31 11:29:41.440773090 +0100
 Birth: -

Any pointers?

-- 
Martin 'Helios' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7

Re: btrfs_inode_item's otime?

[Christoph Hellwig]

On Sat, Jan 10, 2015 at 11:13:52AM +0100, Martin Steigerwald wrote:
> What is the issue with the xstat, new stat format stuff? Basically I have the 
> feeling that this is in discussion for at least 5 years or so and I always 
> wondered where it is stuck. I am not aware of any discussions of it recently, 
> but I may have overseen those.

It's mostly because it turned on into an exercise of blatant
overengineering.  The primary need in the stat area is a few new fields,
and the secondary interesting one is to allow the user to request only
some field that it actually needs (i.e. statlite).  But all the
proposals included bullshit like variable length fields, xattrs and
similar and thus were totally off the table.