Re: [PATCH][RFC] make take_dentry_name_snapshot() lockless

[Jens Axboe <axboe@kernel.dk>]

On 12/22/24 9:25 PM, Al Viro wrote:
> On Tue, Dec 10, 2024 at 02:45:23AM +0000, Al Viro wrote:
>> On Mon, Dec 09, 2024 at 11:12:37PM +0000, Al Viro wrote:
>>>
>>> Actually, grepping for DNAME_INLINE_LEN brings some interesting hits:
>>> drivers/net/ieee802154/adf7242.c:1165:  char debugfs_dir_name[DNAME_INLINE_LEN + 1];
>>> 	cargo-culted, AFAICS; would be better off with a constant of their own.
>>>
>>> fs/ext4/fast_commit.c:326:              fc_dentry->fcd_name.len > DNAME_INLINE_LEN)
>>> fs/ext4/fast_commit.c:452:      if (dentry->d_name.len > DNAME_INLINE_LEN) {
>>> fs/ext4/fast_commit.c:1332:                     fc_dentry->fcd_name.len > DNAME_INLINE_LEN)
>>> fs/ext4/fast_commit.h:113:      unsigned char fcd_iname[DNAME_INLINE_LEN];      /* Dirent name string */
>>> 	Looks like that might want struct name_snapshot, along with
>>> {take,release}_dentry_name_snapshot().
>>
>> See viro/vfs.git#work.dcache.  I've thrown ext4/fast_commit conversion
>> into the end of that pile.  NOTE: that stuff obviously needs profiling.
>> It does survive light testing (boot/ltp/xfstests), but review and more
>> testing (including serious profiling) is obviously needed.
>>
>> Patches in followups...
> 
> More fun with ->d_name, ->d_iname and friends:
> 
> 87ce955b24c9 "io_uring: add ->show_fdinfo() for the io_uring file descriptor"
> is playing silly buggers with ->d_iname for some reason.  This
>         seq_printf(m, "UserFiles:\t%u\n", ctx->file_table.data.nr);
>         for (i = 0; has_lock && i < ctx->file_table.data.nr; i++) {
>                 struct file *f = NULL;
> 
>                 if (ctx->file_table.data.nodes[i])
>                         f = io_slot_file(ctx->file_table.data.nodes[i]);
>                 if (f)
>                         seq_printf(m, "%5u: %s\n", i, file_dentry(f)->d_iname);
>                 else
>                         seq_printf(m, "%5u: <none>\n", i);
>         }
> produces user-visible data.  For each slot in io_uring file table you
> show either that it's empty (fine) or, for files with short names, the
> last component of the name (no quoting, etc. - just a string as-is) or
> the last short name that dentry used to have.
> 
> And that's a user-visible ABI.  What the hell?
> 
> NOTE: file here is may be anything whatsoever.  It may be a pipe,
> an arbitrary file in tmpfs, a socket, etc.
> 
> How hard an ABI it is?  If it's really used by random userland code
> (admin tools, etc.), we have a problem.  If that thing is cast in
> stone, we'll have to emulate the current behaviour of that code,
> no matter what.  I really hope it can be replaced with something
> saner, though.
> 
> Incidentally, call your file "<none>"; is the current behaviour
> the right thing to do?
> 
> What behaviour _is_ actually wanted?  Jens, Jann?

It's not really API, it's just for debugging purposes. Ideal behavior -
show the file name, if possible, if not it can be anything like "anon
inode" or whatever.

IOW, we can change this however we want.

-- 
Jens Axboe