Received-SPF: pass (google.com: best guess record for domain of linux-ext4-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Andreas Dilger <adilger@dilger.ca>
Message-Id: <BFC2DE9E-82FA-4F76-9BD2-B505324F3557@dilger.ca>
Content-Type: multipart/signed;
 boundary="Apple-Mail=_0F36E968-5522-4059-B18F-A5654D9A0571";
 protocol="application/pgp-signature"; micalg=pgp-sha256
Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\))
Subject: Re: [PATCH v2 3/3] ext4: reimplement ext4_empty_dir() using
 is_dirent_block_empty
Date:   Tue, 7 Apr 2020 11:21:01 -0600
In-Reply-To: <20200407064616.221459-3-harshadshirwadkar@gmail.com>
Cc:     linux-ext4@vger.kernel.org
To:     Harshad Shirwadkar <harshadshirwadkar@gmail.com>,
        Jan Kara <jack@suse.cz>
References: <20200407064616.221459-1-harshadshirwadkar@gmail.com>
 <20200407064616.221459-3-harshadshirwadkar@gmail.com>
Sender: linux-ext4-owner@vger.kernel.org
Precedence: bulk


--Apple-Mail=_0F36E968-5522-4059-B18F-A5654D9A0571
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=us-ascii

On Apr 7, 2020, at 12:46 AM, Harshad Shirwadkar =
<harshadshirwadkar@gmail.com> wrote:
>=20
> 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().
>=20
> This is a new patch added in V2.
>=20
> 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 =3D ext4_next_entry(de, sb->s_blocksize);
> +	if (!is_empty_dirent_block(inode, bh, de)) {
> +		brelse(bh);
> +		return false;
> +	}
> +	brelse(bh);
> +
> +	for (lblk =3D 1; lblk < inode->i_size >> =
EXT4_BLOCK_SIZE_BITS(sb);
> +			lblk++) {

(style) should be aligned after '(' on previous line

> +		bh =3D ext4_read_dirblock(inode, lblk, EITHER);
> +		if (bh =3D=3D 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 +=3D ext4_rec_len_from_disk(de->rec_len, =
sb->s_blocksize);
> +		brelse(bh);
> 	}
> -	brelse(bh);
> 	return true;
> }

Cheers, Andreas


--Apple-Mail=_0F36E968-5522-4059-B18F-A5654D9A0571
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename=signature.asc
Content-Type: application/pgp-signature;
	name=signature.asc
Content-Description: Message signed with OpenPGP

-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org

iQIzBAEBCAAdFiEEDb73u6ZejP5ZMprvcqXauRfMH+AFAl6Mtn0ACgkQcqXauRfM
H+Ay3BAAuf9cNqYO6j9OpQk5vuS9lmKkevgiwYH8znhwDmLyjtNkDM4pSbCsPr84
mU8Gyflbqg3zNZbMRVi5Glr6B6ZSMQltXACksE5VGjjGW8uvzNMUtQLwJVvb20OW
SuCJt91Z9BWSUMAcMQwYHLqQrttqcEogiYm10lHPwMUnqnVqSCdjvxtFx+lvK0HJ
nt7N/n1nPBfHuCdH6dgyomGMD0dmIIv28P6ynHqus7MU130z/Wtj22CQn4kzGdeA
an4Mh8pZ7tI5GO5Kf8K8vUtwepAB/ITdDyRgXLsNUqH0nE8qhCxXCeGNs2jY5ToA
AHucg3Gj9mDfQ6wD2IH2Jp6QHoPnVvbku5/oC4pLXE9jO0aTz9VsunMbKRYnz96Z
SNg1gMpo/txXszvcsdGYa/iehQI+qd6hYIDimZcVp6M5E3k4ixwiXjmVgWslgH1U
6xIyNBDiZUgpuqhclhm5zmxjsY/T8+UriWoj6XUb6k8w+knRP68tQ5fLCPYG/Cc7
XbQkJX+kpc/ARj1BjGhx4Mzy3gaez1fZDy+9tA2PCCd+lJtATP8E6CX4CIKrjZR9
ph4xp99uLt+vFmGhiY6k3NaBmnqyT+HUXU1u97Huvbzlr6ovuIXq+dsQRJ0JhHi3
QQ6pWkw/k9bYJA33ipE4TbHJE9HCRC7dTAA07v83qGnGEkAjnOE=
=R+wy
-----END PGP SIGNATURE-----

--Apple-Mail=_0F36E968-5522-4059-B18F-A5654D9A0571--