Prev: [PATCH v3 11/11] KVM: MMU: trace pte prefetch
Next: [PATCH] init: Fix comment
From: Arnd Bergmann on 30 Jun 2010 04:40
On Wednesday 30 June 2010 03:17:12 David Howells wrote:
> +static int xstat_check_param(struct xstat __user *buffer, size_t bufsize,
> + struct kstat *stat)
> +{
> + u32 struct_version;
> + int ret;
> +
> + /* if the buffer isn't large enough, return how much we wanted to
> + * write, but otherwise do nothing */
> + if (bufsize < sizeof(struct xstat))
> + return sizeof(struct xstat);
> +
> + ret = get_user(struct_version, &buffer->struct_version);
> + if (ret < 0)
> + return ret;
> + if (struct_version != 0)
> + return -ENOTSUPP;
> +
> + memset(stat, 0xde, sizeof(*stat));
> +
> + ret = get_user(stat->query_flags, &buffer->query_flags);
> + if (ret < 0)
> + return ret;
> +
> + /* nothing outside this set has a defined purpose */
> + stat->query_flags &= XSTAT_QUERY__DEFINED_SET;
> + stat->result_flags = 0;
> + return 0;
> +}

I think it would be better to leave the structure as write-only from
the kernel and pass the query_flags and struct_version as syscall
arguments, though it makes sense to store them in the result as well.

Independent from this, I also think that we can collapse the
struct_version into the more flexible query_flags. When the structure
gets extended with new fields, just add another flag to let
the user ask for them.

When the flags are outside of the structure, you can even have a flag
that will result in a completely new structure layout to be returned.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 30 Jun 2010 05:00
Arnd Bergmann <arnd(a)arndb.de> wrote:

> I think it would be better to leave the structure as write-only from
> the kernel

Why?

> and pass the query_flags and struct_version as syscall arguments, though it
> makes sense to store them in the result as well.

The problem with that is that the number of syscall arguments is limited, and
there is no SYSCALL_DEFINE7.

On the other hand, I could make a separate argument block struct and pass a
pointer to it...

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Arnd Bergmann on 30 Jun 2010 05:40
On Wednesday 30 June 2010 10:55:51 David Howells wrote:
> Arnd Bergmann <arnd(a)arndb.de> wrote:
>
> > I think it would be better to leave the structure as write-only from
> > the kernel
>
> Why?

Consistency mostly. stat and stat64 don't read it, so I think xstat
also shouldn't if we can easily avoid it.

It also makes things like strace more complicated.

> > and pass the query_flags and struct_version as syscall arguments, though it
> > makes sense to store them in the result as well.
>
> The problem with that is that the number of syscall arguments is limited, and
> there is no SYSCALL_DEFINE7.
>
> On the other hand, I could make a separate argument block struct and pass a
> pointer to it...

No, I think that would be worse than the current version. But if you remove
the structure version in favor of the flags, you only need six arguments
anyway.

You can also go further and fold the structure length into flags, because
the length is just a function of the data you are passing.

Having a system call with flags, size and version is like wearing a belt,
braces and suspenders. An unsigned long flags argument should be enough to
hold up your pants[1].

Arnd

[1] I hope I managed to make this sound wrong in both American and proper
English.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andreas Dilger on 30 Jun 2010 05:40
On 2010-06-29, at 19:48, Trond Myklebust wrote:
>> When the system call is executed, the struct_version ID and query_flags bitmask are read from the buffer to work out what the user is requesting.
>
> Yes, but could we please also add a flag that allows you to specify that
> the kernel _must_ provide up to date attributes.

To my reading, if the query_flags are set in the input buffer, then the attributes MUST be fetched. If they are unset, then they MAY be fetched, and the corresponding query_flags will be set in the return buffer. If the query_flags are not set in the return buffer then I assume the output values are undefined.

In discussions about the proposed "statlite()" API (which this is very similar to) it was desirable that there be separate flags for the individual fields (at least AMC_TIME should be split), since it isn't always clear whether it is "free" to get all of these timestamps, if just one is desired. For Lustre, in particular, the mtime is stored with the file data (where it is updated), and it is more costly to get this if it isn't needed.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: David Howells on 30 Jun 2010 05:50
Andreas Dilger <adilger(a)dilger.ca> wrote:

> > Yes, but could we please also add a flag that allows you to specify that
> > the kernel _must_ provide up to date attributes.
>
> To my reading, if the query_flags are set in the input buffer, then the
> attributes MUST be fetched. If they are unset, then they MAY be fetched,
> and the corresponding query_flags will be set in the return buffer. If the
> query_flags are not set in the return buffer then I assume the output values
> are undefined.

I think Trond may have a point, looking at nfs_getattr().

There can be three levels:

(1) Don't check with the server, just go with what we've got in the cache if
it's available. Results returned may be approximate.

(2) Check with the server if the cached attributes are out of date or if
something is requested that we don't keep in RAM.

(3) Check with the server anyway.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: [PATCH v3 11/11] KVM: MMU: trace pte prefetch
Next: [PATCH] init: Fix comment