Hao Xu wrote on Wed, Jul 12, 2023 at 03:53:24PM +0800: > > > + if (file_count(file) > 1) > > > > I was curious about this so I found it's basically what __fdget_pos does > > before deciding it should take the f_pos_lock, and as such this is > > probably correct... But if someone can chime in here: what guarantees > > someone else won't __fdget_pos (or equivalent through this) the file > > again between this and the vfs_getdents call? > > That second get would make file_count > 1 and it would lock, but lock > > hadn't been taken here so the other call could get the lock without > > waiting and both would process getdents or seek or whatever in > > parallel. > > > > This file_count(file) is atomic_read, so I believe no race condition here. I don't see how that helps in the presence of another thread getting the lock after we possibly issued a getdents without the lock, e.g. t1 call io_uring getdents here t1 sees file_count(file) == 1 and skips getting lock t1 starts issuing vfs_getdents [... processing] t2 calls either io_uring getdents or getdents64 syscall t2 gets the lock, since it wasn't taken by t1 it can be obtained t2 issues another vfs_getdents Christian raised the same issue so I'll leave this to his part of the thread for reply, but I hope that clarified my concern. ----- BTW I forgot to point out: this dropped the REWIND bit from my patch; I believe some form of "seek" is necessary for real applications to make use of this (for example, a web server could keep the fd open in a LRU and keep issuing readdir over and over again everytime it gets an indexing request); not having rewind means it'd need to close and re-open the fd everytime which doesn't seem optimal. A previous iteration discussed that real seek is difficult and not necessarily needed to I settled for rewind, but was there a reason you decided to stop handling that? My very egoistical personal use case won't require it, so I can just say I don't care here, but it would be nice to have a reason explained at some point -- Dominique Martinet | Asmadeus
