Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 23 Jan 2021 15:50:55 -0800
From:   Andres Freund <andres@anarazel.de>
To:     Lennert Buytenhek <buytenh@wantstofly.org>
Cc:     Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
        io-uring@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-btrfs@vger.kernel.org
Subject: Re: [RFC PATCH] io_uring: add support for IORING_OP_GETDENTS64
Message-ID: <20210123235055.azmz5jm2lwyujygc@alap3.anarazel.de>
References: <20210123114152.GA120281@wantstofly.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20210123114152.GA120281@wantstofly.org>
Precedence: bulk

Hi,

On 2021-01-23 13:41:52 +0200, Lennert Buytenhek wrote:
> IORING_OP_GETDENTS64 behaves like getdents64(2) and takes the same
> arguments.

I've wished for this before, this would be awesome.


> One open question is whether IORING_OP_GETDENTS64 should be more like
> pread(2)?and allow passing in a starting offset to read from the
> directory from.  (This would require some more surgery in fs/readdir.c.)

That would imo be preferrable from my end - using the fd's position
means that the fd cannot be shared between threads etc.

It's also not clear to me that right now you'd necessarily get correct
results if multiple IORING_OP_GETDENTS64 for the same fd get processed
in different workers.  Looking at iterate_dir(), it looks to me that the
locking around the file position would end up being insufficient on
filesystems that implement iterate_shared?

int iterate_dir(struct file *file, struct dir_context *ctx)
{
	struct inode *inode = file_inode(file);
	bool shared = false;
	int res = -ENOTDIR;
	if (file->f_op->iterate_shared)
		shared = true;
	else if (!file->f_op->iterate)
		goto out;

	res = security_file_permission(file, MAY_READ);
	if (res)
		goto out;

	if (shared)
		res = down_read_killable(&inode->i_rwsem);
	else
		res = down_write_killable(&inode->i_rwsem);
	if (res)
		goto out;

	res = -ENOENT;
	if (!IS_DEADDIR(inode)) {
		ctx->pos = file->f_pos;
		if (shared)
			res = file->f_op->iterate_shared(file, ctx);
		else
			res = file->f_op->iterate(file, ctx);
		file->f_pos = ctx->pos;
		fsnotify_access(file);
		file_accessed(file);
	}
	if (shared)
		inode_unlock_shared(inode);
	else
		inode_unlock(inode);
out:
	return res;
}

As there's only a shared lock, seems like both would end up with the
same ctx->pos and end up updating f_pos to the same offset (assuming the
same count).

Am I missing something?

Greetings,

Andres Freund