Re: [PATCH v4 0/2] ls: use statx() when it's available

2020-02-03 Thread Pádraig Brady

On 31/10/2019 11:04, Jeff Layton wrote:

On Thu, 2019-10-24 at 10:16 +0100, Pádraig Brady wrote:

On 09/10/2019 22:23, Pádraig Brady wrote:

On 09/10/2019 11:14, Jeff Layton wrote:

On Wed, 2019-10-09 at 10:19 +0100, Pádraig Brady wrote:

On 19/09/19 16:59, Jeff Layton wrote:

v4:
- set appropriate STATX_* bits for time_type, sort_type and
 print_block_size

v3:
- syntax cleanups. make syntax-check now passes

v2:
- add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
 detection
- style cleanups


Sorry for the delay in reviewing.

This looks good, except for the usage
of AT_STATX_DONT_SYNC when retrieving inode info.
Sure that generally doesn't change, but that
would be file system dependent, and I have seen
file systems that populate inode with counters etc.
Anyway that sort of decision would be best done
in the kernel I think, where it would have the
info whether it needs to sync for STATX_INO or not.

OK for me to push without the DONT_SYNC ?



Sure, that seems reasonable. Let me know if you need me to resend.


Pushed without AT_STATX_DONT_SYNC.
Also added the new statx.h to noinst_HEADERS,
and used our _GL_ATTRIBUTE_PURE define rather than
the less portable __attribute__.

https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=a99ab26


FYI I see this is causing issues with docker images (possibly due to seccomp).
https://bugzilla.redhat.com/show_bug.cgi?id=1760300
(There is a bug tracking the kernel issue but it's not publicly accessible.)

  From scanning the comments it seems that once statx() is called
state is messed up, so that having ls fall back from statx() to stat()
wouldn't help in this case. So the assumption that whatever libc providing
statx() does appropriate fallback to stat() is probably still valid.



Yep, this seems like it's probably a seccomp bug. We were going to put
these patches into Fedora 31's coreutils, but reverted the patches until
this is resolved. Hopefully they'll nail down the problem soob.


Jeff I don't have access to the kernel bug tracking this issue:
https://bugzilla.redhat.com/show_bug.cgi?id=1762578
Could you give a summary of the current state?
Am I correct in thinking that if statx() returns 2,
then it would be futile to notice that and fall back to stat()
as state is messed up causing future stat() calls to fail?

thanks,
Pádraig



Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-31 Thread Jeff Layton
On Thu, 2019-10-24 at 10:16 +0100, Pádraig Brady wrote:
> On 09/10/2019 22:23, Pádraig Brady wrote:
> > On 09/10/2019 11:14, Jeff Layton wrote:
> > > On Wed, 2019-10-09 at 10:19 +0100, Pádraig Brady wrote:
> > > > On 19/09/19 16:59, Jeff Layton wrote:
> > > > > v4:
> > > > > - set appropriate STATX_* bits for time_type, sort_type and
> > > > > print_block_size
> > > > > 
> > > > > v3:
> > > > > - syntax cleanups. make syntax-check now passes
> > > > > 
> > > > > v2:
> > > > > - add wrappers for stat_for_ino and fstat_for_ino, don't factor out 
> > > > > loop
> > > > > detection
> > > > > - style cleanups
> > > > 
> > > > Sorry for the delay in reviewing.
> > > > 
> > > > This looks good, except for the usage
> > > > of AT_STATX_DONT_SYNC when retrieving inode info.
> > > > Sure that generally doesn't change, but that
> > > > would be file system dependent, and I have seen
> > > > file systems that populate inode with counters etc.
> > > > Anyway that sort of decision would be best done
> > > > in the kernel I think, where it would have the
> > > > info whether it needs to sync for STATX_INO or not.
> > > > 
> > > > OK for me to push without the DONT_SYNC ?
> > > > 
> > > 
> > > Sure, that seems reasonable. Let me know if you need me to resend.
> > 
> > Pushed without AT_STATX_DONT_SYNC.
> > Also added the new statx.h to noinst_HEADERS,
> > and used our _GL_ATTRIBUTE_PURE define rather than
> > the less portable __attribute__.
> > 
> > https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=a99ab26
> 
> FYI I see this is causing issues with docker images (possibly due to seccomp).
> https://bugzilla.redhat.com/show_bug.cgi?id=1760300
> (There is a bug tracking the kernel issue but it's not publicly accessible.)
> 
>  From scanning the comments it seems that once statx() is called
> state is messed up, so that having ls fall back from statx() to stat()
> wouldn't help in this case. So the assumption that whatever libc providing
> statx() does appropriate fallback to stat() is probably still valid.
> 

Yep, this seems like it's probably a seccomp bug. We were going to put
these patches into Fedora 31's coreutils, but reverted the patches until
this is resolved. Hopefully they'll nail down the problem soob.
-- 
Jeff Layton 




Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-24 Thread Pádraig Brady

On 09/10/2019 22:23, Pádraig Brady wrote:

On 09/10/2019 11:14, Jeff Layton wrote:

On Wed, 2019-10-09 at 10:19 +0100, Pádraig Brady wrote:

On 19/09/19 16:59, Jeff Layton wrote:

v4:
- set appropriate STATX_* bits for time_type, sort_type and
print_block_size

v3:
- syntax cleanups. make syntax-check now passes

v2:
- add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
detection
- style cleanups


Sorry for the delay in reviewing.

This looks good, except for the usage
of AT_STATX_DONT_SYNC when retrieving inode info.
Sure that generally doesn't change, but that
would be file system dependent, and I have seen
file systems that populate inode with counters etc.
Anyway that sort of decision would be best done
in the kernel I think, where it would have the
info whether it needs to sync for STATX_INO or not.

OK for me to push without the DONT_SYNC ?



Sure, that seems reasonable. Let me know if you need me to resend.


Pushed without AT_STATX_DONT_SYNC.
Also added the new statx.h to noinst_HEADERS,
and used our _GL_ATTRIBUTE_PURE define rather than
the less portable __attribute__.

https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=a99ab26


FYI I see this is causing issues with docker images (possibly due to seccomp).
https://bugzilla.redhat.com/show_bug.cgi?id=1760300
(There is a bug tracking the kernel issue but it's not publicly accessible.)

From scanning the comments it seems that once statx() is called
state is messed up, so that having ls fall back from statx() to stat()
wouldn't help in this case. So the assumption that whatever libc providing
statx() does appropriate fallback to stat() is probably still valid.

cheers,
Pádraig



Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-09 Thread Pádraig Brady

On 09/10/2019 11:14, Jeff Layton wrote:

On Wed, 2019-10-09 at 10:19 +0100, Pádraig Brady wrote:

On 19/09/19 16:59, Jeff Layton wrote:

v4:
- set appropriate STATX_* bits for time_type, sort_type and
   print_block_size

v3:
- syntax cleanups. make syntax-check now passes

v2:
- add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
   detection
- style cleanups


Sorry for the delay in reviewing.

This looks good, except for the usage
of AT_STATX_DONT_SYNC when retrieving inode info.
Sure that generally doesn't change, but that
would be file system dependent, and I have seen
file systems that populate inode with counters etc.
Anyway that sort of decision would be best done
in the kernel I think, where it would have the
info whether it needs to sync for STATX_INO or not.

OK for me to push without the DONT_SYNC ?



Sure, that seems reasonable. Let me know if you need me to resend.


Pushed without AT_STATX_DONT_SYNC.
Also added the new statx.h to noinst_HEADERS,
and used our _GL_ATTRIBUTE_PURE define rather than
the less portable __attribute__.

https://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=a99ab26

thanks,
Pádraig



Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-09 Thread Jeff Layton
On Wed, 2019-10-09 at 10:19 +0100, Pádraig Brady wrote:
> On 19/09/19 16:59, Jeff Layton wrote:
> > v4:
> > - set appropriate STATX_* bits for time_type, sort_type and
> >   print_block_size
> > 
> > v3:
> > - syntax cleanups. make syntax-check now passes
> > 
> > v2:
> > - add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
> >   detection
> > - style cleanups
> 
> Sorry for the delay in reviewing.
> 
> This looks good, except for the usage
> of AT_STATX_DONT_SYNC when retrieving inode info.
> Sure that generally doesn't change, but that
> would be file system dependent, and I have seen
> file systems that populate inode with counters etc.
> Anyway that sort of decision would be best done
> in the kernel I think, where it would have the
> info whether it needs to sync for STATX_INO or not.
> 
> OK for me to push without the DONT_SYNC ?
> 

Sure, that seems reasonable. Let me know if you need me to resend.

Thanks,
-- 
Jeff Layton 




Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-09 Thread Pádraig Brady

On 19/09/19 16:59, Jeff Layton wrote:

v4:
- set appropriate STATX_* bits for time_type, sort_type and
  print_block_size

v3:
- syntax cleanups. make syntax-check now passes

v2:
- add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
  detection
- style cleanups


Sorry for the delay in reviewing.

This looks good, except for the usage
of AT_STATX_DONT_SYNC when retrieving inode info.
Sure that generally doesn't change, but that
would be file system dependent, and I have seen
file systems that populate inode with counters etc.
Anyway that sort of decision would be best done
in the kernel I think, where it would have the
info whether it needs to sync for STATX_INO or not.

OK for me to push without the DONT_SYNC ?

thanks,
Pádraig



Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-10-01 Thread Jeff Layton
On Fri, 2019-09-20 at 00:53 +0100, Pádraig Brady wrote:
> This looks good upon quick review.
> I will try to merge over the next couple of days.
> 
> thanks!
> Pádraig

Ping? Any idea when you'll get a chance to review and merge this?

Thanks,
-- 
Jeff Layton 




Re: [PATCH v4 0/2] ls: use statx() when it's available

2019-09-19 Thread Pádraig Brady
This looks good upon quick review.
I will try to merge over the next couple of days.

thanks!
Pádraig



[PATCH v4 0/2] ls: use statx() when it's available

2019-09-19 Thread Jeff Layton
v4:
- set appropriate STATX_* bits for time_type, sort_type and
  print_block_size

v3:
- syntax cleanups. make syntax-check now passes

v2:
- add wrappers for stat_for_ino and fstat_for_ino, don't factor out loop
  detection
- style cleanups

Limiting the distribution list on this posting since the changes from
the last set are very minor.

Original patch description follows:

This patchset converts the ls command to use statx instead of stat when
available. This allows ls to indicate interest in only certain inode
metadata.

This is potentially a win on networked/clustered/distributed
filesystems. In cases where we'd have to do a full, heavyweight stat()
call we can now do a much lighter statx() call.

As a real-world example, consider a filesystem like CephFS where one
client is actively writing to a file and another client does an
ls --color in the same directory. --color means that we need to fetch
the mode of the file.

Doing that with a stat() call means that we have to fetch the size and
mtime in addition to the mode. The MDS in that situation will have to
revoke caps in order to ensure that it has up-to-date values to report,
which disrupts the writer.

This has a measurable affect on performance. I ran a fio sequential
write test on one cephfs client and had a second client do "ls --color"
in a tight loop on the directory that held the file:

Baseline -- no activity on the second client:

  WRITE: bw=76.7MiB/s (80.4MB/s), 76.7MiB/s-76.7MiB/s (80.4MB/s-80.4MB/s), 
io=4600MiB (4824MB), run=60016-60016msec

Without this patch series, we see a noticable performance hit:

  WRITE: bw=70.4MiB/s (73.9MB/s), 70.4MiB/s-70.4MiB/s (73.9MB/s-73.9MB/s), 
io=4228MiB (4433MB), run=60012-60012msec

With this patch series, we gain most of that ground back:

  WRITE: bw=75.9MiB/s (79.6MB/s), 75.9MiB/s-75.9MiB/s (79.6MB/s-79.6MB/s), 
io=4555MiB (4776MB), run=60019-60019msec

Jeff Layton (2):
  stat: move struct statx to struct stat conversion routines to new
header
  ls: use statx instead of stat when available

 src/ls.c| 145 +---
 src/stat.c  |  32 +---
 src/statx.h |  52 +++
 3 files changed, 191 insertions(+), 38 deletions(-)
 create mode 100644 src/statx.h

-- 
2.21.0