Re: [PATCH] io_uring: let IORING_OP_FILES_UPDATE support to choose fixed file slots

[Xiaoguang Wang <xiaoguang.wang@linux.alibaba.com>]

hi,

Thanks for the very quick review :)
> On 5/30/22 6:46 AM, Xiaoguang Wang wrote:
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index 6d91148e9679..58514b8048da 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -5945,16 +5948,20 @@ static int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>>  	return 0;
>>  }
>>  
>> +#define IORING_CLOSE_FD_AND_FILE_SLOT 1
>> +
>>  static int io_close_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>>  {
>> -	if (sqe->off || sqe->addr || sqe->len || sqe->rw_flags || sqe->buf_index)
>> +	if (sqe->off || sqe->addr || sqe->len || sqe->buf_index)
>>  		return -EINVAL;
>>  	if (req->flags & REQ_F_FIXED_FILE)
>>  		return -EBADF;
>>  
>>  	req->close.fd = READ_ONCE(sqe->fd);
>>  	req->close.file_slot = READ_ONCE(sqe->file_index);
>> -	if (req->close.file_slot && req->close.fd)
>> +	req->close.flags = READ_ONCE(sqe->close_flags);
> This needs a:
>
> 	if (req->closeflags & ~IORING_CLOSE_FD_AND_FILE_SLOT)
> 		return -EINVAL;
>
> to be future proof in terms of new flags.
OK.

>
>> +	if (!(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT) &&
>> +	    req->close.file_slot && req->close.fd)
>>  		return -EINVAL;
>>  
>>  	return 0;
>> @@ -5970,7 +5977,8 @@ static int io_close(struct io_kiocb *req, unsigned int issue_flags)
>>  
>>  	if (req->close.file_slot) {
>>  		ret = io_close_fixed(req, issue_flags);
>> -		goto err;
>> +		if (ret || !(req->close.flags & IORING_CLOSE_FD_AND_FILE_SLOT))
>> +			goto err;
>>  	}
>>  
>>  	spin_lock(&files->file_lock);
>> @@ -8003,23 +8011,63 @@ static int io_files_update_prep(struct io_kiocb *req,
>>  	return 0;
>>  }
>>  
>> +static int io_files_update_with_index_alloc(struct io_kiocb *req,
>> +					    unsigned int issue_flags)
>> +{
>> +	__s32 __user *fds = u64_to_user_ptr(req->rsrc_update.arg);
>> +	struct file *file;
>> +	unsigned int done, nr_fds = req->rsrc_update.nr_args;
>> +	int ret, fd;
>> +
>> +	for (done = 0; done < nr_fds; done++) {
>> +		if (copy_from_user(&fd, &fds[done], sizeof(fd))) {
>> +			ret = -EFAULT;
>> +			break;
>> +		}
>> +
>> +		file = fget(fd);
>> +		if (!file) {
>> +			ret = -EBADF;
>> +			goto out;
>> +		}
>> +		ret = io_fixed_fd_install(req, issue_flags, file,
>> +					  IORING_FILE_INDEX_ALLOC);
>> +		if (ret < 0)
>> +			goto out;
>> +		if (copy_to_user(&fds[done], &ret, sizeof(ret))) {
>> +			ret = -EFAULT;
>> +			__io_close_fixed(req, issue_flags, ret);
>> +			break;
>> +		}
>> +	}
>> +
>> +out:
>> +	if (done)
>> +		return done;
>> +	return ret;
>> +}
>> +
>>  static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>>  {
>>  	struct io_ring_ctx *ctx = req->ctx;
>>  	struct io_uring_rsrc_update2 up;
>>  	int ret;
>>  
>> -	up.offset = req->rsrc_update.offset;
>> -	up.data = req->rsrc_update.arg;
>> -	up.nr = 0;
>> -	up.tags = 0;
>> -	up.resv = 0;
>> -	up.resv2 = 0;
>> +	if (req->rsrc_update.offset == IORING_FILE_INDEX_ALLOC) {
>> +		ret = io_files_update_with_index_alloc(req, issue_flags);
>> +	} else {
>> +		up.offset = req->rsrc_update.offset;
>> +		up.data = req->rsrc_update.arg;
>> +		up.nr = 0;
>> +		up.tags = 0;
>> +		up.resv = 0;
>> +		up.resv2 = 0;
> Move 'up' into this branch?
OK.

>
> Do you have a liburing test case for this as well?
Yeah, just sent it.

Regards,
Xiaoguang Wang
>