On 2024/4/26 06:28, Al Viro wrote:
On Fri, Apr 26, 2024 at 05:56:52AM +0800, Gao Xiang wrote:
Hi Al,
On 2024/4/26 04:08, Al Viro wrote:
On Thu, Apr 25, 2024 at 08:56:41PM +0100, Al Viro wrote:
FWIW, see #misc.erofs and #more.erofs in my tree; the former is the
minimal conversion of erofs_read_buf() and switch from buf->inode
to buf->mapping, the latter follows that up with massage for
erofs_read_metabuf().
First two and last four patches resp. BTW, what are the intended rules
for inline symlinks? "Should fit within the same block as the last
symlink on-disk layout follows the same rule of regular files. The last
logical block can be inlined right after the on-disk inode (called tail
packing inline) or use a separate fs block to keep the symlink if tail
packing inline doesn't fit.
byte of on-disk erofs_inode_{compact,extended}"? Feels like
erofs_read_inode() might be better off if it did copying the symlink
body instead of leaving it to erofs_fill_symlink(), complete with
the sanity checks... I'd left that logics alone, though - I'm nowhere
near familiar enough with erofs layout.
If I understand correctly, do you mean just fold erofs_fill_symlink()
into the caller? That is fine with me, I can change this in the
future.
It's just that the calling conventions of erofs_read_inode() feel wrong ;-/
We return a pointer and offset, with (ERR_PTR(...), anything) used to
indicate an error and (pointer into page, offset) used (in case of
fast symlinks and only in case of fast symlinks) to encode the address
of symlink body, with data starting at pointer + offset + vi->xattr_isize
and length being ->i_size, no greater than block size - offset - vi->xattr_size.
If anything, it would be easier to follow (and document) if you had
allocated and filled the symlink body right there in erofs_read_inode().
That way you could lift erofs_put_metabuf() call into erofs_read_inode(),
along with the variable itself.
Perhaps something like void *erofs_read_inode(inode) with
ERR_PTR(-E...) => error
NULL => success, not a fast symlink
pointer to string => success, a fast symlink, body allocated and returned
to caller.
Got it. my original plan was that erofs_read_inode() didn't need
to handle inode->i_mode stuffs (IOWs, different type of inodes).
But I think symlink i_link cases can be handled specifically in
erofs_read_inode().
Or, for that matter, have it return an int and stuff the body into ->i_link -
it's just that you'd need to set ->i_op there with such approach.
Not sure, really. BTW, one comment about erofs_fill_symlink() - it's probably
a good idea to use kmemdup_nul() rather than open-coding it (and do that after
the block overflow check, obviously).
Yes, let me handle all of this later.
Thanks,
Gao Xiang
