Hoist the per-inode invalidation up into a function because we're about
to add invalidating dentries in parent directories. This should result
in no functional change.
Signed-off-by: Zach Brown <zab@versity.com>
We were trying to tear down our mounted file system resources in the
->kill_sb() callback. This happens relatively early in the unmount
process. We call kill_block_super() in our teardown which syncs the
mount and tears down the vfs structures. By tearing down in ->kill_sb()
we were forced to juggle tearing down before and after the call to
kill_block_super().
When we got that wrong we'd tear down too many resources and crash in
kill_block_super() or we wouldn't tear down enough and leave work still
pending that'd explode as we tried to shut down after
kill_block_super().
It turns out the vfs has a callback specifcally to solve this ordering
problem. The put_super callback is called after having synced the mount
but before its totally torn down. By putting all our shutdown in there
we no longer have to worry about racing with active use.
Auditing the shutdown dependencies also found some bad cases where we
were tearding down subsystems that were still in use. The biggest
problem was shutting down locking and networking before shutting down
the transaction processing which relies on both. Now we first shut
down all the client processing, then all the server processing, then the
lowest level common infrastructure.
The trickiest part in understanding this is knowing that
kill_block_super() only calls put_super during mount failure if mount
got far enough to assign the root dentry to s_root. We call put_super
manually ourselves in mount failure if it didn't get far enough so that
all teardown goes through put_super. (You'll see this s_root test in
other upstream file system error paths.)
Finally while auding the setup and shutdown paths I noticed a few, trans
and counters, that needed simple fixes to properly cleanup errors and
only shutdown if they've been setup.
This all was stressed with an xfstests that races mount and unmount
across the cluster. Before this change it'd crash/hang almost instantly
and with this change it runs to completion.
Signed-off-by: Zach Brown <zab@versity.com>
This replaces the fragile recursive locking logic in dlmglue. In particular
that code fails when we have a pending downconvert and a process comes in
for a level that's compatible with the existing level. The downconvert will
still happen which causes us to now believe we are holding a lock that we
are not! We could go back to checking for holders that raced our downconvert
worker but that had problems of its own (see commit e8f7ef0).
Instead of trying to infer from lock state what we are allowed to do, let's
be explicit. Each lock now has a tree of task refs. If you come in to
acquire a lock, we look for our task in that tree. If it's not there, we
know this is the first time this task wanted that lock, so we can continue.
Otherwise we incremement a count on the task ref and return the already
locked lock. Unlock does the opposite - it finds the task ref and decreases
the count. On zero it will proceed with the actual unlock.
The owning task is the only process allowed to manipulate a task ref, so we
only have to lock manipulation of the tree. We make an exception for
global locks which might be unlocked from another process context (in this
case that means the node id lock).
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
We can't have locks with keys that overlap. This adds an rbtree of
locks that are sorted by their key range so that we can find out if we
create overlapping locks before they cause item cache consistency
problems.
Signed-off-by: Zach Brown <zab@versity.com>
Add some _sk suffix variants of the message printing calls so that we
can use per-cpu key buffer arguments without the full SK_PCPU() wrapper.
Signed-off-by: Zach Brown <zab@versity.com>
The mapping of size index item keys to lock names and key ranges was
completely bonkers. Its method of setting variable length masks could
easily create locks with different names whose key ranges overlapped.
We map ranges of sizes to locks and the big change is that all the
inodes in these sizes are covered. We can't try to have groups of
inodes per size because that would result in too many full precision
size locks.
With this fix the size index item locks no longer trigger warnings that
we're creating locks with overlapping keys.
Signed-off-by: Zach Brown <zab@versity.com>
We have to map many index item keys down to a lock that then has a start
and end key range. We also use this mapping over in index item locking
to avoid trying to acquire locks multiple times.
We were duplicating the mapping calculation in these two places. This
refactors these functions to use one range calculation function. It's
going to be used in future patches to fix the mapping of the size index
items.
This should result in no functional changes.
Signed-off-by: Zach Brown <zab@versity.com>
Lock names don't have minor. They're a unique position in
type.major.ino with ino masked to groups. Any index item is mapped to a
single lock.
But then each lock has a range of items that it covers. The index item
key still has a minor from the bad old days of indexing time. When
setting the range of keys covered by the lock name we set it to 0/~0 for
the range.
This is dead wrong because the minor is a higher priority than the inode
in the key space. By setting the minor to 0/~0 we are saying that each
lock name covers *all the minors and inodes for that major*. This is
wrong because there are multiple lock names for different inode groups
for each major. We're in effect having the different lock names
associated with ranges that all overlap.
And this is very bad because it means that a lock can cache keys that
are covered by other locks. An index item lock for a small inode can
accidentally create a negative item cache region for later inodes
covered by an entirely different lock.
We saw failures in scoutfs/500 because of this. A node trying to
read an existing item would get enoent because it had a false negative
cached region from an unrelated lock that overlapped with the lock
that it just acquired from a writer and was trying to read the contents
from.
The fix is to just set the minor to 0. We're not using it. This stops
the lock names with fixed majors and inode ranges from accidentally
overlapping with each other.
Signed-off-by: Zach Brown <zab@versity.com>
Add tracepoints for allocated lock structs entering the tree and finally
being freed. This gives visibility into the lifetime of locks without
using much higher frequency per-operation tracing that blow out other
events.
Signed-off-by: Zach Brown <zab@versity.com>
This was inadvertantly left out of the main CW locking commit. We
simply need to seq_print the new fields. We add them to the end of
the line, thus preserving backwards compatibility with old versions
of the debug format.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
This will give us concurrency yet still allow our ioctls to drive cache
syncing/invalidation on other nodes. Our lock_coverage() checks evolve
to handle direct dlm modes, allowing us to verify correct usage of CW
locks.
As a test, we can run createmany on two nodes at the same time, each
working in their own directory. The following commands were run on each
node:
$ mkdir /scoutfs/`uname -n`
$ cd /scoutfs/`uname -n`
$ /root/createmany -o ./file_$i 100000
Before this patch that test wouldn't finish in any reasonable amount of
time and I would kill it after some number of hours.
After this patch, we make swift progress through the test:
[root@fstest3 fstest3.site]# /root/createmany -o ./file_$i 100000
- created 10000 (time 1509394646.11 total 0.31 last 0.31)
- created 20000 (time 1509394646.38 total 0.59 last 0.28)
- created 30000 (time 1509394646.81 total 1.01 last 0.43)
- created 40000 (time 1509394647.31 total 1.51 last 0.50)
- created 50000 (time 1509394647.82 total 2.02 last 0.51)
- created 60000 (time 1509394648.40 total 2.60 last 0.58)
- created 70000 (time 1509394649.06 total 3.26 last 0.66)
- created 80000 (time 1509394649.72 total 3.93 last 0.66)
- created 90000 (time 1509394650.36 total 4.56 last 0.64)
total: 100000 creates in 35.02 seconds: 2855.80 creates/second
[root@fstest4 fstest4.fstestnet]# /root/createmany -o ./file_$i 100000
- created 10000 (time 1509394647.35 total 0.75 last 0.75)
- created 20000 (time 1509394647.89 total 1.28 last 0.54)
- created 30000 (time 1509394648.46 total 1.86 last 0.58)
- created 40000 (time 1509394648.96 total 2.35 last 0.49)
- created 50000 (time 1509394649.51 total 2.90 last 0.55)
- created 60000 (time 1509394650.07 total 3.46 last 0.56)
- created 70000 (time 1509394650.79 total 4.19 last 0.72)
- created 80000 (time 1509394681.26 total 34.66 last 30.47)
- created 90000 (time 1509394681.63 total 35.03 last 0.37)
total: 100000 creates in 35.50 seconds: 2816.76 creates/second
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
These variants will unconditionally overwrite any existing cached
items, making them appropriate for us with CW locked inode index
items.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
This is a bit trickier than just dropping in a cw holders count.
dlmglue was comparing levels by a simple greater than or less than check.
Since CW locks are not compatible with PR or EX, this check breaks down.
Instead we provide a function which can tell us whether a conversion to a
given lock levels is is compatible (cache-wise) with the level we have.
We also have some slightly more complicated logic in downconvert. As a
result we update the helper that dlmglue uses to choose a downconvert level.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
dlmglue does some holder checks that can become unwieldy, esepcially
with the upcoming CW patch. Put them in a helper function.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
I accidentally left this off with the initial dlmglue commit. I enabled
it here so that I could see our CW locks happening in real time. We
don't print lock name yet but that will be remedied in a future patch.
Turning this on gives us a debugfs file,
/sys/kernel/debug/scoutfs/<fsid>/locking_state
which exports the full lock state to userspace. The information
exported on each lock is extensive. The export includes each locks name
level, blocking level, request state, flags, etc. We also get a count of
lock attempts and failures for each level (cw, pr, ex). In addition we
also get the total time and max time waited on a given lock request.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
I noticed while working on other code that we weren't trying to
free potentially allocated btree iter keys if one of them saw an
allocation failure.
Signed-off-by: Zach Brown <zab@versity.com>
The augmenting of the btree to track items with bits set was too fiddly
for its own good. We were able to migrate old btree blocks with a
simple stored key while also fixing livelocks as the parent and item
bits got out of sync. This is now unused buggy code that can be
removed.
Signed-off-by: Zach Brown <zab@versity.com>
The bit tracking code was a bit much (HA). It introduced a lot of
complexity just to provide a way to migrate blocks from the old
half of the ring into the current half of the ring.
We can get rid of a ton of code and potential for bugs if we simply
store a persistent migration key in the super and use it to
sweep the tree looking for old blocks to dirty. A simple tree walk
that dirties and returns the next key is all we need.
Signed-off-by: Zach Brown <zab@versity.com>
scoutfs_xattr_set() refreshes the cached item inode with its current vfs
inode. It has to refresh its vfs item as it acquires the lock before it
asserts that vfs inode as current.
Signed-off-by: Zach Brown <zab@versity.com>
Expand the generic lock tracing event to trace the level and holders,
add an event for acquiring a lock, and switch the invalidation event
over to using the lock class.
Signed-off-by: Zach Brown <zab@versity.com>
We had callers using the initialization macro, it just didn't do
anything. The uninitialized entries triggered a bug on trying to delete
an uninitialized entry. fsx-mpi tripped over this on shutdown after
seeing a consistency error.
Signed-off-by: Zach Brown <zab@versity.com>
fsx-mpi spins creating contention between ex holders of locks between
nodes. It was tripping assertions in item invalidation as it tried to
invalidate dirty items. Tracing showed that we were allowing holders of
locks while we were invalidating. Our invalidation function would
commit the current transaction, another task would hold the lock and
dirty an item, and then invalidation would continue on and try to
invalidate the dirty item. The invalidation code has always assumed
that it's not running concurrently with item dirtying.
The recursive locking change allowed acquireing blocked locks if the
recursive flag was set. It'd then check holders after calling
downconvert_worker (invalidation for us) and retry the downconvert if a
holder appeared. That it allowed recursive holders regardless of who
was alredy holding the lock is what let holders arrive once downconvert
started on the blocked lock. Not only did this create our problem with
invalidation, it also could leave items behind if the holder dirtied an
item and dropped the lock between invalidation and before downconvert
checked the holders again.
The fix is to only allow recursive holders on blocked locks that already
have holders. This ensures that holders will never increase past zero
on blocked locks. Once the downconvert sees the holders drain it will
call invalidation which won't have racing dirtiers. We can remove the
holder check after invalidation entirely.
With this fixed fsx-mpi no longer tries to invalidate dirty items as it
bounces locks back and forth.
Signed-off-by: Zach Brown <zab@versity.com>
We weren't setting the new flag in the mapped buffer head. This tells
the caller that the buffer is newly allocated and needs to be zeroed.
Without this we expose unwritten newly allocated block contents.
fsx found this almost immediately. With this fixed fsx passes.
Signed-off-by: Zach Brown <zab@versity.com>
Simple attr changes are mostly handled by the VFS, we just have to mirror
them into our inode. Truncates are done in a seperate set of transactions.
We use a flag to indicate an in-progress truncate. This allows us to
detect and continue the truncate should the node crash.
Index locking is a bit complicated, so we add a helper function to grab
index locks and start a transaction.
With this patch we now pass the following xfstests:
generic/014
generic/101
generic/313
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
Call it scoutfs_inode_index_try_lock_hold since it may fail and unwind
as part of normal (not an error) operation. This lets us re-use the
name in an upcoming patch.
Signed-off-by: Mark Fasheh <mfasheh@versity.com>
Renaming a dir between parents and clobbering an existing empty dir
wasn't correctly updating the parent link counts. Updating parent link
counts when dirs are moved between parents is an independent operation
from decreasing the link count of a victim existing target of the
rename.
Signed-off-by: Zach Brown <zab@versity.com>
We only set the .getattr method to our locked getattr filler for regular
files. Set it for all files so that stat, etc, will see the current
inode for all file types.
Signed-off-by: Zach Brown <zab@versity.com>
The fs/dlm code has a harmless but unannotated inversion between
connection and socket locking that triggers during shutdown and disables
lockdep. We don't want it to mask our warnings during testing that may
happen after the first shared unmount so we disable lockdep around the
dlm shutdown. It's not ideal but then neither are distro kernels that
ship with lockdep warnings.
Signed-off-by: Zach Brown <zab@versity.com>
The xattr trans reservation assumed that it was only dirtying items for
the new xattr size. It didn't account for dirty deletion items for
parts from a larger previous xattr.
With this fixed generic/070 no longer triggers warnings.
Signed-off-by: Zach Brown <zab@versity.com>
Add a network greeting message that's exchanged between the client and
server on every connection to make sure that we have the correct file
system and format hash.
Signed-off-by: Zach Brown <zab@versity.com>
Calculate the hash of format.h and ioctl.h and make sure the hash stored
in the super during mkfs matches our calculated hash on mount.
Signed-off-by: Zach Brown <zab@versity.com>
mkfs needs to know the size of the largest btree when figuring out how
big to make the ring. It needs to know how few items we can have in
parent blocks and to know that it needs to know how empty the blocks can
get.
Signed-off-by: Zach Brown <zab@versity.com>
We're going to be strictly enforcing matching format.h and ioctl.h
between userspace and kernel space. Let's get the exported kernel
function definition out of ioctl.h.
Signed-off-by: Zach Brown <zab@versity.com>
All the item ops now know the limit of the items they're allowed to read
into the cache. Warn if someone asks to read items without knowing
how much they're allowed to read based on their lock coverage.
Signed-off-by: Zach Brown <zab@versity.com>
scoutfs_item_create() hasn't been working with lock coverage. It
wouldn't return -ENOENT if it didn't have the lock cached. It would
create items outside lock coverate so they wouldn't be invalidated and
re-read if another node modified the item.
Add a lock arg and teach it to populate the cache so that it's correctly
consistent.
Signed-off-by: Zach Brown <zab@versity.com>
Zach BrownandMark Fasheh
Mark FashehandZach Brown