[2/2] xfs: sync up xfs_trans_inode with userspace
diff mbox series

Message ID 112d2e52-c96c-af83-7e53-5fca12448c76@redhat.com
State Accepted
Headers show
Series
  • xfs: move & sync up xfs_trans_inode with userspace
Related show

Commit Message

Eric Sandeen July 12, 2019, 5:46 p.m. UTC
Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
sync with userspace.  (Currently no kernel caller sends this flag.)

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

Comments

Brian Foster July 15, 2019, 11:30 a.m. UTC | #1
On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
> Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
> sync with userspace.  (Currently no kernel caller sends this flag.)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 93d14e47269d..a9ad90926b87 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
>  		inode->i_mtime = tv;
>  	if (flags & XFS_ICHGTIME_CHG)
>  		inode->i_ctime = tv;
> +	if (flags & XFS_ICHGTIME_CREATE) {
> +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> +	}

Could we add a "for libxfs" or some such one liner comment to this so
long as this is unused in the kernel? With that:

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  }
>  
>  /*
> 
>
Darrick J. Wong July 15, 2019, 3:09 p.m. UTC | #2
On Mon, Jul 15, 2019 at 07:30:08AM -0400, Brian Foster wrote:
> On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
> > Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
> > sync with userspace.  (Currently no kernel caller sends this flag.)
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 93d14e47269d..a9ad90926b87 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
> >  		inode->i_mtime = tv;
> >  	if (flags & XFS_ICHGTIME_CHG)
> >  		inode->i_ctime = tv;
> > +	if (flags & XFS_ICHGTIME_CREATE) {
> > +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> > +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> > +	}
> 
> Could we add a "for libxfs" or some such one liner comment to this so
> long as this is unused in the kernel? With that:

FWIW I was planning (5.4) to fix xfs_ialloc to use ICHGTIME_CREATE, so
it won't be "for libxfs only" for long. :)

--D

> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> >  }
> >  
> >  /*
> > 
> >
Dave Chinner July 15, 2019, 10:22 p.m. UTC | #3
On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
> Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
> sync with userspace.  (Currently no kernel caller sends this flag.)
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 93d14e47269d..a9ad90926b87 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
>  		inode->i_mtime = tv;
>  	if (flags & XFS_ICHGTIME_CHG)
>  		inode->i_ctime = tv;
> +	if (flags & XFS_ICHGTIME_CREATE) {
> +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> +	}

Please use the same format as the rest of the function. i.e.

	if (flags & XFS_ICHGTIME_CREATE)
		ip->i_d.di_crtime = tv;

And convert userspace over to the same :P

Cheers,

Dave.
Darrick J. Wong July 16, 2019, 12:03 a.m. UTC | #4
On Tue, Jul 16, 2019 at 08:22:09AM +1000, Dave Chinner wrote:
> On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
> > Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
> > sync with userspace.  (Currently no kernel caller sends this flag.)
> > 
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> > ---
> > 
> > diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> > index 93d14e47269d..a9ad90926b87 100644
> > --- a/fs/xfs/libxfs/xfs_trans_inode.c
> > +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> > @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
> >  		inode->i_mtime = tv;
> >  	if (flags & XFS_ICHGTIME_CHG)
> >  		inode->i_ctime = tv;
> > +	if (flags & XFS_ICHGTIME_CREATE) {
> > +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
> > +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
> > +	}
> 
> Please use the same format as the rest of the function. i.e.
> 
> 	if (flags & XFS_ICHGTIME_CREATE)
> 		ip->i_d.di_crtime = tv;
> 
> And convert userspace over to the same :P

How about promoting crtime to struct inode while we're at it?

(That said I think Eric was going for the quick resync now to keep
kernel libxfs in sync with xfsprogs libxfs...)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Eric Sandeen July 16, 2019, 12:05 a.m. UTC | #5
On 7/15/19 5:22 PM, Dave Chinner wrote:
> On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
>> Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
>> sync with userspace.  (Currently no kernel caller sends this flag.)
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
>> index 93d14e47269d..a9ad90926b87 100644
>> --- a/fs/xfs/libxfs/xfs_trans_inode.c
>> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
>> @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
>>  		inode->i_mtime = tv;
>>  	if (flags & XFS_ICHGTIME_CHG)
>>  		inode->i_ctime = tv;
>> +	if (flags & XFS_ICHGTIME_CREATE) {
>> +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
>> +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
>> +	}
> 
> Please use the same format as the rest of the function. i.e.
> 
> 	if (flags & XFS_ICHGTIME_CREATE)
> 		ip->i_d.di_crtime = tv;

        struct timespec64 tv;


        struct timespec64       i_atime;
        struct timespec64       i_mtime;
        struct timespec64       i_ctime;

...

        xfs_timestamp_t di_crtime;      /* time created */


bzzzzt.

will take a bit more work I think :(




> And convert userspace over to the same :P
> 
> Cheers,
> 
> Dave.
>
Eric Sandeen July 16, 2019, 12:06 a.m. UTC | #6
On 7/15/19 7:03 PM, Darrick J. Wong wrote:
> On Tue, Jul 16, 2019 at 08:22:09AM +1000, Dave Chinner wrote:
>> On Fri, Jul 12, 2019 at 12:46:17PM -0500, Eric Sandeen wrote:
>>> Add an XFS_ICHGTIME_CREATE case to xfs_trans_ichgtime() to keep in
>>> sync with userspace.  (Currently no kernel caller sends this flag.)
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>> ---
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
>>> index 93d14e47269d..a9ad90926b87 100644
>>> --- a/fs/xfs/libxfs/xfs_trans_inode.c
>>> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
>>> @@ -66,6 +66,10 @@ xfs_trans_ichgtime(
>>>  		inode->i_mtime = tv;
>>>  	if (flags & XFS_ICHGTIME_CHG)
>>>  		inode->i_ctime = tv;
>>> +	if (flags & XFS_ICHGTIME_CREATE) {
>>> +		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
>>> +		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
>>> +	}
>>
>> Please use the same format as the rest of the function. i.e.
>>
>> 	if (flags & XFS_ICHGTIME_CREATE)
>> 		ip->i_d.di_crtime = tv;
>>
>> And convert userspace over to the same :P
> 
> How about promoting crtime to struct inode while we're at it?
> 
> (That said I think Eric was going for the quick resync now to keep
> kernel libxfs in sync with xfsprogs libxfs...)

well, yeah but we don't have to pollute crap just to save 4 lines in a diff.
I'm ok with moving the file and leaving this delta there for now if we need
to work harder on those last 4 lines of diff ;)

-Eric

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index 93d14e47269d..a9ad90926b87 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -66,6 +66,10 @@  xfs_trans_ichgtime(
 		inode->i_mtime = tv;
 	if (flags & XFS_ICHGTIME_CHG)
 		inode->i_ctime = tv;
+	if (flags & XFS_ICHGTIME_CREATE) {
+		ip->i_d.di_crtime.t_sec = (int32_t)tv.tv_sec;
+		ip->i_d.di_crtime.t_nsec = (int32_t)tv.tv_nsec;
+	}
 }
 
 /*