Hi, thanks for such thorough and detailed reviewing and all these
corrections. I will fix them in the next version.
On 4/11/22 9:38 PM, David Howells wrote:
> Jeffle Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
>
>> + (*) On-demand Read.
>> +
>
> Unnecessary extra blank line.
>
> Jeffle Xu <jefflexu@xxxxxxxxxxxxxxxxx> wrote:
>
> What's the scope of the uniqueness of "id"? Is it just unique to a particular
> cachefiles cache?
Yes. Currently each cache, I mean, each "struct cachefiles_cache",
maintains an xarray. The id is unique in the scope of the cache.
>
>> +
>> + struct cachefiles_close {
>> + __u32 fd;
>> + };
>> +
>
> "where:"
>
>> + * ``fd`` identifies the anon_fd to be closed, which is exactly the same
>
> "... which should be the same as that provided to the OPEN request".
>
> Is it possible for userspace to move the fd around with dup() or whatever?
Currently No. The anon_fd is stored in
```
struct cachefiles_object {
int fd;
...
}
```
When sending READ/CLOSE request, the associated anon_fd is all fetched
from @fd field of struct cachefiles_object. dup() won't update @fd field
of struct cachefiles_object.
Thus when dup() is done, let's say there are fd A (original) and fd B
(duplicated from fd A) associated to the cachefiles_object. Then the @fd
field of following READ/CLOSE requests is always fd A, since @fd field
of struct cachefiles_object is not updated. However the CREAD (reply to
READ request) ioctl indeed can be done on either fd A or fd B.
Then when fd A is closed while fd B is still alive, @fd field of
following READ/CLOSE requests is still fd A, which is indeed buggy since
fd A can be reused then.
To fix this, I plan to replace @fd field of READ/CLOSE requests with
@object_id field.
```
struct cachefiles_close {
__u32 object_id;
};
struct cachefiles_read {
__u32 object_id;
__u64 off;
__u64 len;
};
```
Then each cachefiles_object has a unique object_id (in the scope of
cachefiles_cache). Each object_id can be mapped to multiple fds (1:N
mapping), while kernel only send an initial fd of this object_id through
OPEN request.
```
struct cachefiles_open {
__u32 object_id;
__u32 fd;
__u32 volume_key_size;
__u32 cookie_key_size;
__u32 flags;
__u8 data[];
};
```
The user daemon can modify the mapping through dup(), but it's
responsible for maintaining and updating this mapping. That is, the
mapping between object_id and all its associated fds should be
maintained in the user space.
>> +
>> + struct cachefiles_read {
>> + __u64 off;
>> + __u64 len;
>> + __u32 fd;
>> + };
>> +
>> + * ``off`` identifies the starting offset of the requested file range.
>
> identifies -> indicates
>
>> +
>> + * ``len`` identifies the length of the requested file range.
>> +
>
> identifies -> indicates (you could alternatively say "specified")
>
>> + * ``fd`` identifies the anonymous fd of the requested cache file. It is
>> + guaranteed that it shall be the same with
>
> "same with" -> "same as"
>
> Since the kernel cannot make such a guarantee, I think you may need to restate
> this as something like "Userspace must present the same fd as was given in the
> previous OPEN request".
Yes, whether the @fd field of READ request is same as that of OPEN
request or not, is actually implementation dependent. However as
described above, I'm going to change @fd field into @object_id field.
After that refactoring, the @object_id field of READ/CLOSE request
should be the same as the @object_id filed of CLOSE request.
>> +CACHEFILES_IOC_CREAD ioctl on the corresponding anon_fd::
>> +
>> + ioctl(fd, CACHEFILES_IOC_CREAD, id);
>> +
>> + * ``fd`` is exactly the fd field of the previous READ request.
>
> Does that have to be true? What if userspace moves it somewhere else?
>
As described above, I'm going to change @fd field into @object_id field.
Then there is an @object_id filed in READ request. When replying the
READ request, the user daemon itself needs to get the corresponding
anon_fd of the given @object_id through the self-maintained mapping.
--
Thanks,
Jeffle
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
