Hi Dan,
Thanks for your kindly review.
On 2019/1/30 22:45, Dan Carpenter wrote:
On Tue, Jan 29, 2019 at 11:55:40PM +0800, Gao Xiang wrote:
+static struct page *find_target_block_classic(struct inode *dir,
struct erofs_qstr *name,int *_diff,int *_ndirents){ unsigned int startprfx, endprfx;
- unsigned int head, back;
- int head, back; struct address_space *const mapping = dir->i_mapping; struct page *candidate = ERR_PTR(-ENOENT);
@@ -105,33 +108,34 @@ static struct page *find_target_block_classic( back = inode_datablocks(dir) - 1; while (head <= back) {
unsigned int mid = head + (back - head) / 2;
const int mid = head + (back - head) / 2;
if (IS_ERR(page)) {-exact_out:
if (!IS_ERR(candidate)) /* valid candidate */put_page(candidate);return page;} else {int diff;unsigned int ndirents, matched;struct qstr dname;
if (!IS_ERR(page)) {It's almost always better to do failure handling instead of success handing because it lets you pull everything in one indent level. You'd need to move a bunch of the declarations around.
I just want to leave definition and the initial assignment in one line...
struct erofs_dirent *de = kmap_atomic(page);
unsigned int nameoff = le16_to_cpu(de->nameoff);ndirents = nameoff / sizeof(*de);
const int nameoff = nameoff_from_disk(de->nameoff,EROFS_BLKSIZ);const int ndirents = nameoff / sizeof(*de);
or I have to unsigned int mid = head + (back - head) / 2; const int mid = head + (back - head) / 2; struct page *page = read_mapping_page(mapping, mid, NULL); struct erofs_dirent *de; ... int ndirents;
if (IS_ERR(page)) { ... }
de = kmap_atomic(page); ... ndirents = nameoff / sizeof(*de);
which takes extra lines...
if (IS_ERR(page)) goto out;
But really the out label is not part of the loop so you could move it to the bottom of the function...
It seems that the out label is the part of loop...
struct erofs_dirent *de = kmap_atomic(page);
unsigned int nameoff = le16_to_cpu(de->nameoff);ndirents = nameoff / sizeof(*de);
const int nameoff = nameoff_from_disk(de->nameoff,EROFS_BLKSIZ);const int ndirents = nameoff / sizeof(*de);int diff;unsigned int matched;struct erofs_qstr dname;
/* corrupted dir (should have one entry at least) */BUG_ON(!ndirents || nameoff > PAGE_SIZE);
if (unlikely(!ndirents)) {DBG_BUGON(1);put_page(page);page = ERR_PTR(-EIO);goto out;We need to kunmap_atomic(de) on this path.
Thanks, will fix in the next version...
}matched = min(startprfx, endprfx); dname.name = (u8 *)de + nameoff;
dname.len = ndirents == 1 ?/* since the rest of the last page is 0 */EROFS_BLKSIZ - nameoff: le16_to_cpu(de[1].nameoff) - nameoff;
if (ndirents == 1)dname.end = (u8 *)de + EROFS_BLKSIZ;elsedname.end = (u8 *)de +nameoff_from_disk(de[1].nameoff,EROFS_BLKSIZ);/* string comparison without already matched prefix */ diff = dirnamecmp(name, &dname, &matched); @@ -139,7 +143,7 @@ static struct page *find_target_block_classic( if (unlikely(!diff)) { *_diff = 0;
goto exact_out;
goto out; } else if (diff > 0) { head = mid + 1; startprfx = matched;@@ -147,35 +151,42 @@ static struct page *find_target_block_classic( if (likely(!IS_ERR(candidate)))
^^^^^^Not related to the this patch, but I wonder how this works. IS_ERR() already has an opposite unlikely() inside so I wonder which trumps the other?
Yes, you are right. That is a remaining issue in the original code. I will set up a clean up patch to fix that.
Thanks, Gao Xiang
put_page(candidate); candidate = page;
*_ndirents = ndirents;regards, dan carpenter