On 2021/6/15 下午11:58, Johannes Thumshirn wrote:

On 15/06/2021 14:18, Qu Wenruo wrote:

quoted

+static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
+					unsigned int opf, bio_end_io_t endio_func)
+{
+	struct bio *bio;
+
+	bio = btrfs_bio_alloc(disk_bytenr);
+	/* bioset allocation should not fail */
+	ASSERT(bio);

Here you write that bio allocation shouldn't fail (because it's backed
by a bioset/mempool and we're not calling from IRQ context).

But alloc_compressed_bio() has other error path, namingly
btrfs_get_chunk_map() and btrfs_get_io_geometry() can fail, thus caller
still need to check that.

Although thanks to your mention, I find that I should call bio_put() for
the above error cases before returning ERR_CAST().

Thanks,
Qu

[...]

quoted

+	bio = alloc_compressed_bio(cb, first_byte, bio_op | write_flags,
+				   end_compressed_bio_write);
+	if (IS_ERR(bio)) {
+		kfree(cb);
+		return errno_to_blk_status(PTR_ERR(bio));
  	}

Here you're checking for IS_ERR().

quoted

@@ -545,10 +569,14 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,

[...]

quoted

+			bio = alloc_compressed_bio(cb, first_byte,
+					bio_op | write_flags,
+					end_compressed_bio_write);
+			if (IS_ERR(bio)) {
+				ret = errno_to_blk_status(PTR_ERR(bio));
+				bio = NULL;
+				goto finish_cb;
+			}

same

quoted

@@ -812,10 +840,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,

[...]

quoted

+	comp_bio = alloc_compressed_bio(cb, cur_disk_byte, REQ_OP_READ,
+					end_compressed_bio_read);
+	if (IS_ERR(comp_bio)) {
+		ret = errno_to_blk_status(PTR_ERR(comp_bio));
+		comp_bio = NULL;
+		goto fail2;
+	}

same

if btrfs_bio_alloc() would have failed we'd already crash on a nullptr
dereference much earlier.