[PATCH] xfs: Set xfs_buf's b_ops member when zeroing bitmap/summary files

[PATCH] xfs: Set xfs_buf's b_ops member when zeroing bitmap/summary files

[Chandan Babu R]

In xfs_growfs_rt(), we enlarge bitmap and summary files by allocating
new blocks for both files. For each of the new blocks allocated, we
allocate an xfs_buf, zero the payload, log the contents and commit the
transaction. Hence these buffers will eventually find themselves
appended to list at xfs_ail->ail_buf_list.

Later, xfs_growfs_rt() loops across all of the new blocks belonging to
the bitmap inode to set the bitmap values to 1. In doing so, it
allocates a new transaction and invokes the following sequence of
functions,
  - xfs_rtfree_range()
    - xfs_rtmodify_range()
      - xfs_rtbuf_get()
        We pass '&xfs_rtbuf_ops' as the ops pointer to xfs_trans_read_buf().
        - xfs_trans_read_buf()
	  We find the xfs_buf of interest in per-ag hash table, invoke
	  xfs_buf_reverify() which ends up assigning '&xfs_rtbuf_ops' to
	  xfs_buf->b_ops.

On the other hand, if xfs_growfs_rt_alloc() had allocated a few blocks
for the bitmap inode and returned with an error, all the xfs_bufs
corresponding to the new bitmap blocks that have been allocated would
continue to be on xfs_ail->ail_buf_list list without ever having a
non-NULL value assigned to their b_ops members. An AIL flush operation
would then trigger the following warning message to be printed on the
console,

  XFS (loop0): _xfs_buf_ioapply: no buf ops on daddr 0x58 len 8
  00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
  CPU: 3 PID: 449 Comm: xfsaild/loop0 Not tainted 5.8.0-rc4-chandan-00038-g4d8c2b9de9ab-dirty #37
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
  Call Trace:
   dump_stack+0x57/0x70
   _xfs_buf_ioapply+0x37c/0x3b0
   ? xfs_rw_bdev+0x1e0/0x1e0
   ? xfs_buf_delwri_submit_buffers+0xd4/0x210
   __xfs_buf_submit+0x6d/0x1f0
   xfs_buf_delwri_submit_buffers+0xd4/0x210
   xfsaild+0x2c8/0x9e0
   ? __switch_to_asm+0x42/0x70
   ? xfs_trans_ail_cursor_first+0x80/0x80
   kthread+0xfe/0x140
   ? kthread_park+0x90/0x90
   ret_from_fork+0x22/0x30

This message indicates that the xfs_buf had its b_ops member set to
NULL.

This commit fixes the issue by assigning "&xfs_rtbuf_ops" to b_ops
member of each of the xfs_bufs logged by xfs_growfs_rt_alloc().

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---

PS: I had got xfs_growfs_rt_alloc() to return an error code after
allocating a few blocks by injecting an error tag from userspace. This
was done to test "Prevent inode extent overflow" patchset. The error
injection causes xfs_growfs_rt_alloc() to return -EFBIG error code
when an inode's extent count goes beyond a certain threshold.

 fs/xfs/xfs_rtalloc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index ace99cfa3e8b..9d4e33d70d2a 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -849,6 +849,7 @@ xfs_growfs_rt_alloc(
 				goto out_trans_cancel;
 
 			xfs_trans_buf_set_type(tp, bp, buf_type);
+			bp->b_ops = &xfs_rtbuf_ops;
 			memset(bp->b_addr, 0, mp->m_sb.sb_blocksize);
 			xfs_trans_log_buf(tp, bp, 0, mp->m_sb.sb_blocksize - 1);
 			/*
-- 
2.28.0

Re: [PATCH] xfs: Set xfs_buf's b_ops member when zeroing bitmap/summary files

[Christoph Hellwig]

On Thu, Sep 17, 2020 at 03:13:56PM +0530, Chandan Babu R wrote:
> In xfs_growfs_rt(), we enlarge bitmap and summary files by allocating
> new blocks for both files. For each of the new blocks allocated, we
> allocate an xfs_buf, zero the payload, log the contents and commit the
> transaction. Hence these buffers will eventually find themselves
> appended to list at xfs_ail->ail_buf_list.
> 
> Later, xfs_growfs_rt() loops across all of the new blocks belonging to
> the bitmap inode to set the bitmap values to 1. In doing so, it
> allocates a new transaction and invokes the following sequence of
> functions,
>   - xfs_rtfree_range()
>     - xfs_rtmodify_range()
>       - xfs_rtbuf_get()
>         We pass '&xfs_rtbuf_ops' as the ops pointer to xfs_trans_read_buf().
>         - xfs_trans_read_buf()
> 	  We find the xfs_buf of interest in per-ag hash table, invoke
> 	  xfs_buf_reverify() which ends up assigning '&xfs_rtbuf_ops' to
> 	  xfs_buf->b_ops.
> 
> On the other hand, if xfs_growfs_rt_alloc() had allocated a few blocks
> for the bitmap inode and returned with an error, all the xfs_bufs
> corresponding to the new bitmap blocks that have been allocated would
> continue to be on xfs_ail->ail_buf_list list without ever having a
> non-NULL value assigned to their b_ops members. An AIL flush operation
> would then trigger the following warning message to be printed on the
> console,
> 
>   XFS (loop0): _xfs_buf_ioapply: no buf ops on daddr 0x58 len 8
>   00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   00000070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   CPU: 3 PID: 449 Comm: xfsaild/loop0 Not tainted 5.8.0-rc4-chandan-00038-g4d8c2b9de9ab-dirty #37
>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014
>   Call Trace:
>    dump_stack+0x57/0x70
>    _xfs_buf_ioapply+0x37c/0x3b0
>    ? xfs_rw_bdev+0x1e0/0x1e0
>    ? xfs_buf_delwri_submit_buffers+0xd4/0x210
>    __xfs_buf_submit+0x6d/0x1f0
>    xfs_buf_delwri_submit_buffers+0xd4/0x210
>    xfsaild+0x2c8/0x9e0
>    ? __switch_to_asm+0x42/0x70
>    ? xfs_trans_ail_cursor_first+0x80/0x80
>    kthread+0xfe/0x140
>    ? kthread_park+0x90/0x90
>    ret_from_fork+0x22/0x30
> 
> This message indicates that the xfs_buf had its b_ops member set to
> NULL.
> 
> This commit fixes the issue by assigning "&xfs_rtbuf_ops" to b_ops
> member of each of the xfs_bufs logged by xfs_growfs_rt_alloc().
> 
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>