Messages in this thread | | | Date | Wed, 30 Mar 2022 13:23:23 +1100 | Subject | Re: [RESEND PATCH v7 7/8] kernfs: Replace per-fs rwsem with hashed rwsems. | From | Imran Khan <> |
| |
Hello Al, Hello Tejun,
On 22/3/22 1:40 pm, Al Viro wrote: > On Mon, Mar 21, 2022 at 09:20:06AM -1000, Tejun Heo wrote: >> Hello, >> >> On Mon, Mar 21, 2022 at 05:55:53PM +0000, Al Viro wrote: >>> Why bother with rwsem, when we don't need anything blocking under it? >>> DEFINE_RWLOCK instead of DEFINE_SPINLOCK and don't make it static. >> >> Oh I mean, in case the common readers get way too hot, percpu_rwsem is a >> relatively easy way to shift the burder from the readers to the writers. I >> doubt we'll need that. >> >>> kernfs_walk_ns() - this is fucking insane; on the surface, it needs to >>> be exclusive due to the use of the same static buffer. It uses that >>> buffer to generate a pathname, *THEN* walks over it with strsep(). >>> That's an... interesting approach, for the lack of other printable >>> terms - we walk the chain of ancestors, concatenating their names >>> into a buffer and separating those names with slashes, then we walk >>> that buffer, searching for slashes... WTF? >> >> It takes the @parent to walk string @path from. Where does it generate the >> pathname? > > Sorry, misread that thing - the reason it copies the damn thing at all is > the use of strsep(). Yecch... Rule of the thumb regarding strsep() use, > be it in kernel or in the userland: don't. It's almost never the right > primitive to use. > > Lookups should use qstr; it has both the length and place for hash. > Switch kernfs_find_ns() to that (and lift the calculation of length > into the callers that do not have it - note that kernfs_iop_lookup() > does) and you don't need the strsep() shite (or copying) anymore. > > That would allow for kernfs_walk_ns() to take kernfs_rename_lock shared. > > HOWEVER, that's not the only lock needed there and this patchset is > broken in that respect - it locks the starting node, then walks the > path. Complete with lookups in rbtrees of children in the descendents > of that node and those are *not* locked. > Yes. This was wrong. I have tried to fix it by dropping the lock of previous parent and taking the lock of current parent before each invocation of kernfs_find_ns from kernfs_walk_ns. However this does not look feasible because we are already under spinlock (kernfs_rename_lock). This limitation will still be there even after changing kernfs_rename_lock to a read-write lock. I have thought of ways to fix this but have not yet got any solution. I am checking further but in the mean time if you have some suggestions please let me know.
Thanks -- Imran
| |