| Message ID | 20200407064616.221459-3-harshadshirwadkar@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] ext4: return lblk from ext4_find_entry | expand |
On Apr 7, 2020, at 12:46 AM, Harshad Shirwadkar <harshadshirwadkar@gmail.com> wrote: > > The new function added in this patchset adds an efficient way to > check if a dirent block is empty. Based on that, reimplement > ext4_empty_dir(). > > This is a new patch added in V2. > > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> Again, a very minor style change iff patch is refreshed. Reviewed-by: Andreas Dilger <adilger@dilger.ca> While looking at this code, I noticed that ext4_empty_dir() considers a directory without a "." or ".." entry to be empty. I see this was changed in 64d4ce8923 ("ext4: fix ext4_empty_dir() for directories with holes"). I can understand that we want to not die on corrupted leaf blocks, but it isn't clear to me that it is a good idea to allow deleting an entire directory tree if the first block has an error (missing "." or ".." as the first and second entries) but is otherwise valid. There were definitely bugs in the past that made "." or ".." not be the first and second entries. That isn't a problem in this patch, but seems dangerous in general. It doesn't seem like the directory shrinking would trigger this case, since it is checking for individual empty blocks rather than using ext4_empty_dir(). > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > @@ -3218,34 +3219,26 @@ bool ext4_empty_dir(struct inode *inode) > brelse(bh); > return true; > } > + de = ext4_next_entry(de, sb->s_blocksize); > + if (!is_empty_dirent_block(inode, bh, de)) { > + brelse(bh); > + return false; > + } > + brelse(bh); > + > + for (lblk = 1; lblk < inode->i_size >> EXT4_BLOCK_SIZE_BITS(sb); > + lblk++) { (style) should be aligned after '(' on previous line > + bh = ext4_read_dirblock(inode, lblk, EITHER); > + if (bh == NULL) > continue; > - } > - if (le32_to_cpu(de->inode)) { > + if (IS_ERR(bh)) > + return true; > + if (!is_empty_dirent_block(inode, bh, NULL)) { > brelse(bh); > return false; > } > - offset += ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize); > + brelse(bh); > } > - brelse(bh); > return true; > } Cheers, Andreas
On Tue 07-04-20 11:21:01, Andreas Dilger wrote: > While looking at this code, I noticed that ext4_empty_dir() considers a > directory without a "." or ".." entry to be empty. I see this was changed > in 64d4ce8923 ("ext4: fix ext4_empty_dir() for directories with holes"). > I can understand that we want to not die on corrupted leaf blocks, but it > isn't clear to me that it is a good idea to allow deleting an entire > directory tree if the first block has an error (missing "." or ".." as the > first and second entries) but is otherwise valid. There were definitely > bugs in the past that made "." or ".." not be the first and second entries. That's a good question. I'd just say that ext4_empty_dir() generally returns true when there's some problem with the directory. In commit 64d4ce8923 I just followed that convention. This behavior of ext4_empty_dir() (and empty_dir() before in ext3) dates back at least to the beginning of git history... I guess we could err on the safer side and disallow directory deletion if there is any problem with it but I guess there was some motivation for this behavior in the past? Maybe somebody remembers? Honza
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 39360c442dad..dae7d15fba5c 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -3179,6 +3179,7 @@ bool ext4_empty_dir(struct inode *inode) struct buffer_head *bh; struct ext4_dir_entry_2 *de; struct super_block *sb; + ext4_lblk_t lblk; if (ext4_has_inline_data(inode)) { int has_inline_data = 1; @@ -3218,34 +3219,26 @@ bool ext4_empty_dir(struct inode *inode) brelse(bh); return true; } - offset += ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize); - while (offset < inode->i_size) { - if (!(offset & (sb->s_blocksize - 1))) { - unsigned int lblock; - brelse(bh); - lblock = offset >> EXT4_BLOCK_SIZE_BITS(sb); - bh = ext4_read_dirblock(inode, lblock, EITHER); - if (bh == NULL) { - offset += sb->s_blocksize; - continue; - } - if (IS_ERR(bh)) - return true; - } - de = (struct ext4_dir_entry_2 *) (bh->b_data + - (offset & (sb->s_blocksize - 1))); - if (ext4_check_dir_entry(inode, NULL, de, bh, - bh->b_data, bh->b_size, offset)) { - offset = (offset | (sb->s_blocksize - 1)) + 1; + de = ext4_next_entry(de, sb->s_blocksize); + if (!is_empty_dirent_block(inode, bh, de)) { + brelse(bh); + return false; + } + brelse(bh); + + for (lblk = 1; lblk < inode->i_size >> EXT4_BLOCK_SIZE_BITS(sb); + lblk++) { + bh = ext4_read_dirblock(inode, lblk, EITHER); + if (bh == NULL) continue; - } - if (le32_to_cpu(de->inode)) { + if (IS_ERR(bh)) + return true; + if (!is_empty_dirent_block(inode, bh, NULL)) { brelse(bh); return false; } - offset += ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize); + brelse(bh); } - brelse(bh); return true; }
The new function added in this patchset adds an efficient way to check if a dirent block is empty. Based on that, reimplement ext4_empty_dir(). This is a new patch added in V2. Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@gmail.com> --- fs/ext4/namei.c | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-)