Ceph » Linux kernel client

I think we need to make ceph_uninline_data idempotent, such that if it gets called a second time it becomes a no-op.

My initial thinking was to use the page lock of the first page in the cache to serialize the callers, since we do that already anyway, but there are some cases where we pass this function page pointers that are not in the cache, or let it allocate a page on its own to do the uninlining. That means that racing tasks could end up operating on different pages, so that scheme may not be possible.

I really don't want to add another mutex to the inode, so I'm considering serializing the callers on the i_truncate_mutex. We'd typically only need to take it on the slow path anyway, so that may be acceptable.

For ceph_write_iter(), it's protected by i_rwsem. But in ceph_page_mkwrite, i_rwsem is not locked.

Not exactly.

After the uninlining is done, the i_inline_version is not updated until much later, after the i_rwsem is dropped. Thus, you can have one task in write_iter do the uninlining and then drop the i_rwsem and do the write, and then have another task do the uninlining again, overwriting the write done by the first task.

I think we have to ensure that some sort of lock is held during the uninlining and that the i_inline_version is updated before we drop it. Then, racing tasks can re-check the version after they get the lock but before they do the uninlining.

Patchset posted upstream:

https://marc.info/?l=ceph-devel&m=156277513921656&w=2

My latest version of this set seems to have uncovered a ABBA deadlock in the inline write handling code. ceph_fill_inline_data takes the s_mutex and then the page lock, but ceph_page_mkwrite ends up doing the reverse. We'll need to fix this up in the context of this set.

Looking over the code some more, I think the main issue is that we have so much of this code running under the session->s_mutex. The code just says:

        struct mutex      s_mutex;    /* serialize session messages */          

...but it's held even when we're not dealing with message ordering. It's not 100% clear what this lock protects.

I think we need to look at reducing the footprint of the s_mutex. If we can get to where ceph_fill_inline_data runs without it, then that would fix this deadlock (and probably have performance benefits as well).

More breadcrumbs -- from handle_reply():

/*
 * Handle mds reply.
 *
 * We take the session mutex and parse and process the reply immediately.
 * This preserves the logical ordering of replies, capabilities, etc., sent
 * by the MDS as they are applied to our local cache.
 */

...I think this rationale is somewhat bogus though.

There is a 1:1 relationship between a ceph_connection and ceph_mds_session. The dispatcher for the connection runs in workqueue context, so only a single dispatch() call for a connection will be running at a time. Therefore the reply handlers are already serialized by virtue of running under the workqueue. I think the s_mutex is superfluous here.