On Mon, 11 Jul 2011 13:37:57 +0200
Edward Shishkin <edward(a)redhat.com> wrote:
> More specifically, the lease operations involve a
"magic" extended
> attribute of the form trusted.crypt.OFFSET-LENGTH-HEAD-TAIL, where
> the fields in the last part have the following meanings:
>
> OFFSET is the beginning of the write in the next step,
> shifted left if unaligned
>
> LENGTH is the length of the write in the next step, shifted
> right if unaligned
>
> HEAD is the number of bytes for the server to return at the
> beginning of the range defined by OFFSET and LENGTH
>
> TAIL is the number of bytes for the server to return at the
> end of the range defined by OFFSET and LENGTH
It seems we came to an agreement that passing such "normal
parameters" is meaningless: since we implement file leasing,
it is impossible to construct a unique lease id by them..
I don't think whole-file leases would work. It's entirely possible and
valid for other parts of GlusterFS (such as write-behind) to issue
concurrent writes on the same connection and fd. If we over-serialize
them, we throw away that parallelism - and we can ill afford to reduce
performance any more than is already necessary. The server already
knows the server and fd for a request; it is those *in addition to* the
offset and length that uniquely identify a write being performed as
part of an atomic read-modify-write sequence.
I think client should pass a 128-bit generation counter. And
server should append (or prepends) a unique client id.
There is no way for the client to pass either of these back as part of
the write, so that the write can be associated with the earlier lease
operation. Changing the network protocol to be incompatible with
"standard" GlusterFS is not a realistic option at this point, so we
have to make that association based on the current write parameters.
> The lease-acquire operation is an fgetxattr.
Why?
I find its signature rather improper for this:
typedef int32_t (*fop_fgetxattr_t) (call_frame_t *frame,
xlator_t *this,
fd_t *fd,
const char *name);
How are we going to pass the magic name and encoded value
via ->fgetxattr()?
I suggest to consider ->fsetxattr() for this purpose..
There is no way to get data *back* from server to client with
(f)setxattr. All we get is a status, so to get the unaligned start/end
pieces we'd need to incur yet another network round trip. With
(f)fgetxattr we can pass data both in (via the xattr name) and out with
a single round trip.
> The write operation must exactly match the HEAD and OFFSET
values
> in
order to
> be considered a continuation of the same
> read-modify-sequence;
otherwise, it
> will be considered a separate write and queued behind the sequence
already in
> progress.
>
> The lease-relinquish operation
Do we really need the above?
oplock->writev_cbk(), and oplock->truncate_cbk can drop
the lease. We must know the atom size on the server side
though (to recognize RMWs). But IMHO this is not a
problem (we can store this in xattrs (**) for example..)
Knowing the atom size is not enough; we need to know the actual end of
the user's write - possibly the new EOF, and possibly not the same as
the end of the write request due to padding. That said, if we included
this information as part of the initial fgetxattr then we might not
need the final fsetxattr.
> Reads must also be aware of this protocol, similarly to aligned
writes. When
> a read is received which conflicts with an in-progress write or
> write
sequence,
> it is queued and continued when the write completes.
I don't see any reasons why reads must be aware of this
protocol. What problems are resolved by making them aware
of this? We do know what are conflicting writes, but what
are conflicting read and write?
Atomicity (of the read-modify-write sequence) requires that
inconsistent intermediate states not be observable by other requests.
Between the time a writev actually hits the disk on the server and the
time we finish doing EOF-related fixups we're in such an inconsistent
state. If we're extending the file and the read is near the new EOF,
this could result in a read receiving more data than is actually
supposed to be in the file. If we're extending the file and the read
is near the *old* EOF, which requires moving the old end fragment from
an xattr to its "proper" location after the old EOF, then the read
could get garbage. If we eliminate the fsetxattr and do the EOF fixups
first, then we're back to the "read more than actually exists" problem
at the old EOF. One way or another, the read needs to wait for *all
parts* of an in-progress read-modify-write before it can proceed.
> * End-fragment xattrs should be removed when they're no
> longer valid,
This makes things queasy. Every such removal can cause rebalance of
local-fs storage. Could you please remind why to not have files with
rounded-up sizes on server? After all we can keep their actual size
in xattrs and to not bother with its removal...
With that approach, we'd have to intercept every lookup, stat, etc. to
ensure that we return the correct st_size. Similarly, every read would
have to check the xattr value to ensure that it doesn't return data
past EOF even though the underlying local FS will. We can cache the
"virtual EOF" to avoid some of these xattr fetches, but at the cost of
more complex interlocking between the read and write paths to make sure
the value remains consistent and up to date. It's a viable approach,
but it mostly trades one set of problems for another and my guess was
that the overhead would end up being greater than the way we have now.
The important thing at this point, with the Fedora 16 code-freeze
deadline approaching, is to have a protocol that is actually proven -
ideally through actual implementation and testing - to address the few
dozen non-obvious race conditions and such that have already been
uncovered by this first implementation. If and when such an
alternative implementation actually exists, we can do an empirical
comparison of overhead and fault-handling characteristics, then choose
whichever is better. Given how far behind we've become in this area, I
don't think it's possible for that to happen as part of the current
release.