On Tue, 25 Jun 2024 04:33:00 -0700, Christoph Hellwig wrote:
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -2526,6 +2526,8 @@ xlog_write(
> > xlog_write_full(lv, ticket, iclog, &log_offset,
> > &len, &record_cnt, &data_cnt);
> > }
> > + if (lv->lv_flags & XFS_LOG_VEC_DYNAMIC)
> > + kvfree(lv->lv_iovecp);
>
> This should porbably be a function paramter to xlog_write, with
> xlog_cil_write_chain asking for the iovecs to be freed because they
> are dynamically allocated, and the other two not becaue the iovecs
> are on-stack. With that we don't need to grow a new field in
> struct xfs_log_vec.
xlog_write() will write all xfs_log_iovec on the lv chain linked list to iclog.
We seem to have no way to distinguish whether the xfs_log_iovec on the lv_chain
list is on the stack by adding new parameters to xlog_write().
>
> > list_for_each_entry(lip, &tp->t_items, li_trans) {
> > struct xfs_log_vec *lv;
> > + struct xfs_log_iovec *lvec;
> > int niovecs = 0;
> > int nbytes = 0;
> > int buf_size;
> > @@ -339,18 +339,23 @@ xlog_cil_alloc_shadow_bufs(
> > * the buffer, only the log vector header and the iovec
> > * storage.
> > */
> > - kvfree(lip->li_lv_shadow);
> > - lv = xlog_kvmalloc(buf_size);
> > -
> > - memset(lv, 0, xlog_cil_iovec_space(niovecs));
> > + if (lip->li_lv_shadow) {
> > + kvfree(lip->li_lv_shadow->lv_iovecp);
> > + kvfree(lip->li_lv_shadow);
> > + }
> > + lv = xlog_kvmalloc(sizeof(struct xfs_log_vec));
> > + memset(lv, 0, sizeof(struct xfs_log_vec));
> > + lvec = xlog_kvmalloc(buf_size);
> > + memset(lvec, 0, xlog_cil_iovec_space(niovecs));
>
> This area can use quite a bit of a redo. The xfs_log_vec is tiny,
> so it doesn't really need a vmalloc fallback but can simply use
> kmalloc.
>
> But more importantly there is no need to really it, you just
> need to allocate it. So this should probably become:
>
> lv = lip->li_lv_shadow;
> if (!lv) {
> /* kmalloc and initialize, set lv_size to zero */
> }
>
> if (buf_size > lv->lv_size) {
> /* grow case that rallocates ->lv_iovecp */
> } else {
> /* same or smaller, optimise common overwrite case */
> ..
> }
If we take the memory allocation of xfs_log_vec out of the if branch below, we
have to face the corner case of buf_size = 0.
But the release and reallocation of xfs_log_vec in this patch is indeed
redundant. I've optimized it in [PATCH v2] and [PATCH v3].
Link to [PATCH v3]:
- https://lore.kernel.org/linux-xfs/20240626044909.15060-1-alexjlzheng@xxxxxxxxxxx/T/#t
Thank you for your suggestion. :)
Jinliang Zheng
