| Message ID | 20240812075659.1399447-1-mjguzik@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | close_files(): reimplement based on do_close_on_exec() | expand |
On Mon, Aug 12, 2024 at 09:56:58AM +0200, Mateusz Guzik wrote: > While here take more advantage of the fact nobody should be messing with > the table anymore and don't clear the fd slot. > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > --- > > how about this instead, I think it's a nicer clean up. > It's literally do_close_on_exec except locking and put fd are deleted. TBH, I don't see much benefit that way - if anything, you are doing a bunch of extra READ_ONCE() of the same thing (files->fdt), for no visible reason...
On Wed, Aug 14, 2024 at 7:24 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Mon, Aug 12, 2024 at 09:56:58AM +0200, Mateusz Guzik wrote: > > While here take more advantage of the fact nobody should be messing with > > the table anymore and don't clear the fd slot. > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > --- > > > > how about this instead, I think it's a nicer clean up. > > > It's literally do_close_on_exec except locking and put fd are deleted. > > TBH, I don't see much benefit that way - if anything, you are doing > a bunch of extra READ_ONCE() of the same thing (files->fdt), for no > visible reason... I claim the stock code avoidably implements traversal differently from do_close_on_exec. The fdt reload can be trivially lifted out of the loop, does not affect what I was going for. But now that you mention this can also be done in the do_close_on_exec case -- the thread calling it is supposed to be the only consumer, so fdt can't change. that's my $0,03 here, I'm not going to further argue about it
diff --git a/fs/file.c b/fs/file.c index 74d7ad676579..3ff2e8265156 100644 --- a/fs/file.c +++ b/fs/file.c @@ -389,33 +389,38 @@ struct files_struct *dup_fd(struct files_struct *oldf, unsigned int max_fds) return newf; } -static struct fdtable *close_files(struct files_struct * files) +static struct fdtable *close_files(struct files_struct *files) { /* * It is safe to dereference the fd table without RCU or * ->file_lock because this is the last reference to the * files structure. + * + * For the same reason we can skip locking. */ struct fdtable *fdt = rcu_dereference_raw(files->fdt); - unsigned int i, j = 0; + unsigned i; - for (;;) { + for (i = 0; ; i++) { unsigned long set; - i = j * BITS_PER_LONG; - if (i >= fdt->max_fds) + unsigned fd = i * BITS_PER_LONG; + fdt = files_fdtable(files); + if (fd >= fdt->max_fds) break; - set = fdt->open_fds[j++]; - while (set) { - if (set & 1) { - struct file * file = xchg(&fdt->fd[i], NULL); - if (file) { - filp_close(file, files); - cond_resched(); - } - } - i++; - set >>= 1; + set = fdt->open_fds[i]; + if (!set) + continue; + for ( ; set ; fd++, set >>= 1) { + struct file *file; + if (!(set & 1)) + continue; + file = fdt->fd[fd]; + if (!file) + continue; + filp_close(file, files); + cond_resched(); } + } return fdt;
While here take more advantage of the fact nobody should be messing with the table anymore and don't clear the fd slot. Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> --- how about this instead, I think it's a nicer clean up. It's literally do_close_on_exec except locking and put fd are deleted. boots & does not blow up, but admittedly I did not bother with ltp or any serious testing fs/file.c | 37 +++++++++++++++++++++---------------- 1 file changed, 21 insertions(+), 16 deletions(-)