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.
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).
