bulkstat fixups

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses
xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different
semantics.
xfs_bulkstat() will return the next inode after the one supplied
while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will only
lookup the
inode supplied and return an error if it is an internal inode.

- in xfs_bulkstat(), need to initialise 'lastino' to the inode supplied so in cases
were we return without examining any inodes the scan wont restart
back at zero.

- sanity check for valid *ubcountp values. Cannot sanity check for valid ubuffer
here because some users of xfs_bulkstat() don't supply a buffer.

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The
mixture of
checks against statstruct_size and 0 was one of the reasons we were
skipping
inodes.

- if the formatter function returns BULKSTAT_RV_NOTHING and an error and the error
is not ENOENT or EINVAL then we need to abort the scan. ENOENT is
for inodes that
are no longer valid and we just skip them. EINVAL is returned if we
try to lookup
an internal inode so we skip them too. For a DMF scan if the inode
and DMF
attribute cannot fit into the space left in the user's buffer it
would return
ERANGE. We didn't handle this error and skipped the inode. We
would continue to
skip inodes until one fitted into the user's buffer or we completed
the scan.

- put back the recalculation of agino (that got removed with the last fix) at the
end of the while loop. This is because the code at the start of the
loop expects
agino to be the last inode examined if it is non-zero.

- if we found some inodes but then encountered an error, return success this time
and the error next time. If the formatter aborted with ENOMEM we
will now return
this error but only if we couldn't read any inodes. Previously if
we encountered
ENOMEM without reading any inodes we returned a zero count and no
error which
falsely indicated the scan was complete.

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different semantics. xfs_bulkstat() will return the next inode after the one supplied while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will only lookup the
inode supplied and return an error if it is an internal inode.

- in xfs_bulkstat(), need to initialise 'lastino' to the inode supplied so in cases
were we return without examining any inodes the scan wont restart back at zero.

- sanity check for valid *ubcountp values. Cannot sanity check for valid ubuffer
here because some users of xfs_bulkstat() don't supply a buffer.

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The mixture of
checks against statstruct_size and 0 was one of the reasons we were skipping
inodes.

- if the formatter function returns BULKSTAT_RV_NOTHING and an error and the error
is not ENOENT or EINVAL then we need to abort the scan. ENOENT is for inodes that
are no longer valid and we just skip them. EINVAL is returned if we try to lookup

an internal inode so we skip them too. For a DMF scan if the inode and DMF
attribute cannot fit into the space left in the user's buffer it would return
ERANGE. We didn't handle this error and skipped the inode. We would continue to
skip inodes until one fitted into the user's buffer or we completed the scan.

- put back the recalculation of agino (that got removed with the last fix) at the
end of the while loop. This is because the code at the start of the loop expects
agino to be the last inode examined if it is non-zero.

- if we found some inodes but then encountered an error, return success this time
and the error next time. If the formatter aborted with ENOMEM we will now return
this error but only if we couldn't read any inodes. Previously if we encountered
ENOMEM without reading any inodes we returned a zero count and no error which
falsely indicated the scan was complete.

[Lachlan, can you wrap your email text at 72 columns for ease of quoting?]

Quoted 3 lines · Lachlan McIlroy

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

OK.

Quoted 10 lines

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different semantics. xfs_bulkstat() will return the next inode after the one supplied while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will only lookup the
inode supplied and return an error if it is an internal inode.

Userspace visile change. What applications do we have that rely on this behaviour that will be broken by this change?

Quoted 5 lines

- in xfs_bulkstat(), need to initialise 'lastino' to the inode supplied so in cases
were we return without examining any inodes the scan wont restart back at zero.

Ok.

Quoted 3 lines

- sanity check for valid *ubcountp values. Cannot sanity check for valid ubuffer
here because some users of xfs_bulkstat() don't supply a buffer.

Ok - the cases that supply a buffer are caught by the first checks you added, right?

Quoted 7 lines

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The mixture of
checks against statstruct_size and 0 was one of the reasons we were skipping
inodes.

Can you wrap these checks in a static inline function so that it is obvious what the correct way to check is and we don't reintroduce this porblem? i.e.

static inline int
xfs_bulkstat_ubuffer_large_enough(ssize_t space)

{
	return (space > sizeof(struct blah));
}

That will also remove a stack variable....

Quoted 15 lines

- if the formatter function returns BULKSTAT_RV_NOTHING and an error and the error
is not ENOENT or EINVAL then we need to abort the scan. ENOENT is for inodes that
are no longer valid and we just skip them. EINVAL is returned if we try to lookup
an internal inode so we skip them too. For a DMF scan if the inode and DMF
attribute cannot fit into the space left in the user's buffer it would return
ERANGE. We didn't handle this error and skipped the inode. We would continue to
skip inodes until one fitted into the user's buffer or we completed the scan.

ok.

Quoted 5 lines

- put back the recalculation of agino (that got removed with the last fix) at the
end of the while loop. This is because the code at the start of the loop expects
agino to be the last inode examined if it is non-zero.

ok.

Quoted 9 lines

- if we found some inodes but then encountered an error, return success this time
and the error next time. If the formatter aborted with ENOMEM we will now return
this error but only if we couldn't read any inodes. Previously if we encountered
ENOMEM without reading any inodes we returned a zero count and no error which
falsely indicated the scan was complete.

ok.

FWIW - missing from this set of patches - cpu_relax() in the loops. In the case where no I/O is required to do the scan, we can hold the cpu for a long time and that will hold off I/O completion, etc for the cpu bulkstat is running on. Hence after every cluster we scan we should cpu_relax() to allow other processes cpu time on that cpu.

Quoted 21 lines · David Chinner

[Lachlan, can you wrap your email text at 72 columns for ease of quoting?]

Quoted 3 lines · Lachlan McIlroy

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

OK.

Quoted 11 lines

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different semantics.
xfs_bulkstat() will return the next inode after the one supplied while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will only lookup the
inode supplied and return an error if it is an internal inode.

Quoted 2 lines

Userspace visile change. What applications do we have that rely on this behaviour that will be broken by this change?

Any apps that rely on the existing behaviour are probably broken. If an app wants to call xfs_bulkstat_single() it should use XFS_IOC_FSBULKSTAT_SINGLE.

Quoted 18 lines

Quoted 7 lines

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The mixture of
checks against statstruct_size and 0 was one of the reasons we were skipping
inodes.

Can you wrap these checks in a static inline function so that it is obvious what the correct way to check is and we don't reintroduce this porblem? i.e.

static inline int
xfs_bulkstat_ubuffer_large_enough(ssize_t space)

{
	return (space > sizeof(struct blah));
}

Quoted 1 line

That will also remove a stack variable....

That won't work - statstruct_size is passed into xfs_bulkstat() so we don't know what 'blah' is. Maybe a macro would be easier.

#define XFS_BULKSTAT_UBLEFT (ubleft >= statstruct_size)

Quoted 9 lines

FWIW - missing from this set of patches - cpu_relax() in the loops. In the case
where no I/O is required to do the scan, we can hold the cpu for a long time
and that will hold off I/O completion, etc for the cpu bulkstat is running on.
Hence after every cluster we scan we should cpu_relax() to allow other processes cpu time on that cpu.

I don't get how cpu_relax() works. I see that it is called at times with a spinlock held so it wont trigger a context switch. Does it give interrupts a chance to run?

It appears to be used where a minor delay is needed - I don't think we have any
cases in xfs_bulkstat() where we need to wait for an event that isn't I/O.

Quoted 22 lines · Lachlan McIlroy

Quoted 21 lines · David Chinner

[Lachlan, can you wrap your email text at 72 columns for ease of quoting?]

Quoted 3 lines · Lachlan McIlroy

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

OK.

Quoted 11 lines

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different semantics.
xfs_bulkstat() will return the next inode after the one supplied while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will only lookup the
inode supplied and return an error if it is an internal inode.

Userspace visile change. What applications do we have that rely on this behaviour that will be broken by this change?

Quoted 2 lines

Any apps that rely on the existing behaviour are probably broken. If an app wants to call xfs_bulkstat_single() it should use XFS_IOC_FSBULKSTAT_SINGLE.

Perhaps, but we can't arbitrarily decide that those apps will now break on a new kernel with this change. At minimum we need to audit all of the code we have that uses bulkstat for such breakage (including DMF!) before we make a change like this.

Quoted 17 lines

Quoted 17 lines

Quoted 8 lines

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The mixture of
checks against statstruct_size and 0 was one of the reasons we were skipping
inodes.

Can you wrap these checks in a static inline function so that it is obvious what the correct way to check is and we don't reintroduce this porblem? i.e.

static inline int
xfs_bulkstat_ubuffer_large_enough(ssize_t space)

{
	return (space > sizeof(struct blah));
}

Quoted 5 lines

Quoted 1 line

That will also remove a stack variable....

That won't work - statstruct_size is passed into xfs_bulkstat() so we don't know what 'blah' is. Maybe a macro would be easier.

#define XFS_BULKSTAT_UBLEFT (ubleft >= statstruct_size)

Yeah, something like that, but I don't like macros with no parameters used like that....

Quoted 8 lines

Quoted 6 lines

FWIW - missing from this set of patches - cpu_relax() in the loops. In the case where no I/O is required to do the scan, we can hold the cpu for a long time and that will hold off I/O completion, etc for the cpu bulkstat is running on. Hence after every cluster we scan we should cpu_relax() to allow other processes cpu time on that cpu.

I don't get how cpu_relax() works. I see that it is called at times with a spinlock held so it wont trigger a context switch. Does it give interrupts a chance to run?

Sorry, my mistake - confused cpu_relax() with cond_resched(). take the above paragraph and s/cpu_relax/cond_resched/g

Quoted 2 lines

It appears to be used where a minor delay is needed - I don't think we have any cases in xfs_bulkstat() where we need to wait for an event that isn't I/O.

The issue is when we're hitting cached buffers and we never end up waiting for I/O - we will then monopolise the cpu we are running on and hold off all other processing. It's antisocial and leads to high latencies for other code.

Cheers,

Dave.

Updated patch - I added cond_resched() calls into each loop - for loops that have a 'continue' somewhere in them I added the cond_resched() at the start, otherwise I put it at the end.

Quoted 23 lines · David Chinner

Quoted 22 lines · Lachlan McIlroy

Quoted 21 lines · David Chinner

[Lachlan, can you wrap your email text at 72 columns for ease of quoting?]

Quoted 3 lines · Lachlan McIlroy

Here's a collection of fixups for bulkstat for all the remaining issues.

- sanity check for NULL user buffer in xfs_ioc_bulkstat[_compat]()

OK.

Quoted 11 lines

- remove the special case for XFS_IOC_FSBULKSTAT with count == 1. This special
case causes bulkstat to fail because the special case uses xfs_bulkstat_single()
instead of xfs_bulkstat() and the two functions have different semantics.
xfs_bulkstat() will return the next inode after the one supplied
while skipping
internal inodes (ie quota inodes). xfs_bulkstate_single() will
only lookup the
inode supplied and return an error if it is an internal inode.

Userspace visile change. What applications do we have that rely on this behaviour that will be broken by this change?

Quoted 7 lines

Quoted 2 lines

Any apps that rely on the existing behaviour are probably broken. If an app wants to call xfs_bulkstat_single() it should use XFS_IOC_FSBULKSTAT_SINGLE.

Perhaps, but we can't arbitrarily decide that those apps will now break on a new kernel with this change. At minimum we need to audit all of the code we have that uses bulkstat for such breakage (including DMF!) before we make a change like this.

I've looked through everything we have in xfs-cmds and nothing relies on this bug being present. Vlad helped me with the DMF side - DMF does not use the XFS_IOC_FSBULKSTAT ioctl, it has it's own interface into the kernel which calls xfs_bulkstat() directly so it wont be affected by this change.

Quoted 49 lines

Quoted 17 lines

Quoted 17 lines

Quoted 8 lines

- checks against 'ubleft' (the space left in the user's buffer) should be against
'statstruct_size' which is the supplied minimum object size. The mixture of
checks against statstruct_size and 0 was one of the reasons we
were skipping
inodes.

Can you wrap these checks in a static inline function so that it is obvious what the correct way to check is and we don't reintroduce this porblem? i.e.

static inline int
xfs_bulkstat_ubuffer_large_enough(ssize_t space)

{
    return (space > sizeof(struct blah));
}

Quoted 5 lines

Quoted 1 line

That will also remove a stack variable....

That won't work - statstruct_size is passed into xfs_bulkstat() so we don't know what 'blah' is. Maybe a macro would be easier.

#define XFS_BULKSTAT_UBLEFT (ubleft >= statstruct_size)

Yeah, something like that, but I don't like macros with no parameters used like that....

Quoted 8 lines

Quoted 6 lines

FWIW - missing from this set of patches - cpu_relax() in the loops. In the case where no I/O is required to do the scan, we can hold the cpu for a long time and that will hold off I/O completion, etc for the cpu bulkstat is running on. Hence after every cluster we scan we should cpu_relax() to allow other processes cpu time on that cpu.

I don't get how cpu_relax() works. I see that it is called at times with a spinlock held so it wont trigger a context switch. Does it give interrupts a chance to run?

Sorry, my mistake - confused cpu_relax() with cond_resched(). take the above paragraph and s/cpu_relax/cond_resched/g

Quoted 2 lines

It appears to be used where a minor delay is needed - I don't think we have any cases in xfs_bulkstat() where we need to wait for an event that isn't I/O.

The issue is when we're hitting cached buffers and we never end up waiting for I/O - we will then monopolise the cpu we are running on and hold off all other processing. It's antisocial and leads to high latencies for other code.

Cheers,

Dave.

Updated patch - I added cond_resched() calls into each loop - for loops that have a 'continue' somewhere in them I added the cond_resched() at the start, otherwise I put it at the end.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

Userspace visile change. What applications do we have that rely on this behaviour that will be broken by this change?

Any apps that rely on the existing behaviour are probably broken. If an app
wants to call xfs_bulkstat_single() it should use XFS_IOC_FSBULKSTAT_SINGLE.

Quoted 5 lines

Perhaps, but we can't arbitrarily decide that those apps will now break on a new kernel with this change. At minimum we need to audit all of the code we have that uses bulkstat for such breakage (including DMF!) before we make a
change like this.

I've looked through everything we have in xfs-cmds and nothing relies on this bug being present. Vlad helped me with the DMF side - DMF does not use the XFS_IOC_FSBULKSTAT ioctl, it has it's own interface into the kernel which calls xfs_bulkstat() directly so it wont be affected by this change.

xfs/xfs_itable.c renamed from xfs/xfs_itable.c_1.157 +2 -0

+#define XFS_BULKSTAT_UBLEFT(ubleft)	((ubleft) >= statstruct_size)
+

I don't think this macro is really helpful. An inline would have been useful if statstruct_size was constant, but this way it's much better to just write out the comparism the four times it's used.

xfs/xfs_itable.c renamed from xfs/xfs_itable.c_1.157 +3 -0

+	if (!ubcountp || *ubcountp <= 0) {
+		return EINVAL;
+	}

No need for the braces here.

I also must say I don't like the cond_resched() calls very much. They look entirely random to me. We really should only need cond_resched when it's absolutely needed, and it deserves a comment why it's needed then.