Hi Christoph, On Mon, Sep 02, 2019 at 05:46:45AM -0700, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 01:51:09PM +0800, Gao Xiang wrote: > > Hi, > > > > This patchset is based on the following patch by Pratik Shinde, > > https://lore.kernel.org/linux-erofs/20190830095615.10995-1-pratikshinde320@xxxxxxxxx/ > > > > All patches addressing Christoph's comments on v6, which are trivial, > > most deleted code are from erofs specific fault injection, which was > > followed f2fs and previously discussed in earlier topic [1], but > > let's follow what Christoph's said now. > > I like where the cleanups are going. But this is really just basic For now, I think it will go to Greg's tree. Before that, I will do patchset v2 to address all remaining comments... > code quality stuff. We're not addressing the issues with large amounts > of functionality duplicating VFS helpers. You means killing erofs_get_meta_page or avoid erofs_read_raw_page? - For killing erofs_get_meta_page, here is the current erofs_get_meta_page: 35 struct page *erofs_get_meta_page(struct super_block *sb, erofs_blk_t blkaddr) 36 { 37 struct inode *const bd_inode = sb->s_bdev->bd_inode; 38 struct address_space *const mapping = bd_inode->i_mapping; 39 /* prefer retrying in the allocator to blindly looping below */ 40 const gfp_t gfp = mapping_gfp_constraint(mapping, ~__GFP_FS); 41 struct page *page; 42 int err; 43 44 repeat: 45 page = find_or_create_page(mapping, blkaddr, gfp); 46 if (!page) 47 return ERR_PTR(-ENOMEM); 48 49 DBG_BUGON(!PageLocked(page)); 50 51 if (!PageUptodate(page)) { 52 struct bio *bio; 53 54 bio = erofs_grab_bio(sb, REQ_OP_READ | REQ_META, 55 blkaddr, 1, sb, erofs_readendio); 56 57 if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) { 58 err = -EFAULT; 59 goto err_out; 60 } 61 62 submit_bio(bio); 63 lock_page(page); 64 65 /* this page has been truncated by others */ 66 if (page->mapping != mapping) { 67 unlock_page(page); 68 put_page(page); 69 goto repeat; 70 } 71 72 /* more likely a read error */ 73 if (!PageUptodate(page)) { 74 err = -EIO; 75 goto err_out; 76 } 77 } 78 return page; 79 80 err_out: 81 unlock_page(page); 82 put_page(page); 83 return ERR_PTR(err); 84 } I think it is simple enough. read_cache_page need write a similar filler, or read_cache_page_gfp will call .readpage, and then introduce buffer_heads, that is what I'd like to avoid now (no need these bd_inode buffer_heads in memory...) - For erofs_read_raw_page, it can be avoided after iomap tail-end packing feature is done... If we remove it now, it will make EROFS broken. It is no urgent and Chao will focus on iomap tail-end packing feature. Thanks, Gao Xiang _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
