Hey Jan! On Thu, May 20, 2021 at 03:55:27PM +0200, Jan Kara wrote: > On Thu 20-05-21 12:09:45, Matthew Bobrowski wrote: > > Hey Jan/Amir/Christian, > > > > This is the updated patch series for adding pidfd support to the > > fanotify API. It incorporates all the suggestions that had come out of > > the initial RFC patch series [0]. > > > > The main difference with this patch series is that FAN_REPORT_PIDFD > > results in an additional info record object supplied alongside the > > generic event metadata object instead of overloading metadata->pid. If > > any of the fid flavoured init flags are specified, then the pidfd info > > record object will follow any fid info record objects. > > > > [0] https://www.spinics.net/lists/linux-fsdevel/msg193296.html > > Overall the series looks fine to me - modulo the problems Christian & Amir > found. Do you have any tests for this? Preferably for LTP so that we can > extend the coverage there? Cool and thanks for glancing over this series. I've written some simple programs to verify this functionality works in FID and non-FID modes. I definitely plan on writing LTP tests, although it's something I'll do once we've agreed on the approach and I've received an ACK from yourself, Amir and Christian. This series passes all current LTP regressions. Also, I guess I'll need to write some patches for man-pages given this is an ABI change. There's one thing that I'd like to mention, and it's something in regards to the overall approach we've taken that I'm not particularly happy about and I'd like to hear all your thoughts. Basically, with this approach the pidfd creation is done only once an event has been queued and the notification worker wakes up and picks up the event from the queue processes it. There's a subtle latency introduced when taking such an approach which at times leads to pidfd creation failures. As in, by the time pidfd_create() is called the struct pid has already been reaped, which then results in FAN_NOPIDFD being returned in the pidfd info record. Having said that, I'm wondering what the thoughts are on doing pidfd creation earlier on i.e. in the event allocation stages? This way, the struct pid is pinned earlier on and rather than FAN_NOPIDFD being returned in the pidfd info record because the struct pid has been already reaped, userspace application will atleast receive a valid pidfd which can be used to check whether the process still exists or not. I think it'll just set the expectation better from an API perspective. /M
