Re: [PATCH 04/11] ntfs: write compressed data before replacing old clusters

2026=EB=85=84 7=EC=9B=94 21=EC=9D=BC (=ED=99=94) =EC=98=A4=ED=9B=84 6:54, N=
amjae Jeon <[email protected]>=EB=8B=98=EC=9D=B4 =EC=9E=91=EC=84=B1:
>
> ntfs_write_cb() punches the old compression unit and publishes the new
> mapping before submitting the replacement data. An allocation or I/O
> failure after the punch loses the previous contents and can leave the
> mapping pointing at unwritten clusters.
>
> Allocate and write the replacement clusters first. Replace the runlist on=
ly
> after the synchronous write succeeds, and free new clusters on failure.
>
> Signed-off-by: Namjae Jeon <[email protected]>

Looks good to me.

Reviewed-by: Hyunchul Lee <[email protected]>


> ---
>  fs/ntfs/compress.c | 63 ++++++++++++++++++++++++++--------------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/fs/ntfs/compress.c b/fs/ntfs/compress.c
> index 33ed0456bf7e..5b5cd494e5d8 100644
> --- a/fs/ntfs/compress.c
> +++ b/fs/ntfs/compress.c
> @@ -1373,10 +1373,6 @@ static int ntfs_write_cb(struct ntfs_inode *ni, lo=
ff_t pos, struct page **pages,
>
>         new_length =3D ntfs_bytes_to_cluster(vol, round_up(bio_size, vol-=
>cluster_size));
>
> -       err =3D ntfs_non_resident_attr_punch_hole(ni, new_vcn, ni->itype.=
compressed.block_clusters);
> -       if (err < 0)
> -               goto out;
> -
>         rlc =3D ntfs_cluster_alloc(vol, new_vcn, new_length, -1, DATA_ZON=
E,
>                         false, true, true);
>         if (IS_ERR(rlc)) {
> @@ -1385,28 +1381,6 @@ static int ntfs_write_cb(struct ntfs_inode *ni, lo=
ff_t pos, struct page **pages,
>         }
>
>         bio_lcn =3D rlc->lcn;
> -       down_write(&ni->runlist.lock);
> -       rl =3D ntfs_runlists_merge(&ni->runlist, rlc, 0, &new_rl_count);
> -       if (IS_ERR(rl)) {
> -               up_write(&ni->runlist.lock);
> -               ntfs_error(vol->sb, "Failed to merge runlists");
> -               err =3D PTR_ERR(rl);
> -               if (ntfs_cluster_free_from_rl(vol, rlc))
> -                       ntfs_error(vol->sb, "Failed to free hot clusters.=
");
> -               kvfree(rlc);
> -               goto out;
> -       }
> -
> -       ni->runlist.count =3D new_rl_count;
> -       ni->runlist.rl =3D rl;
> -
> -       err =3D ntfs_attr_update_mapping_pairs(ni, 0);
> -       up_write(&ni->runlist.lock);
> -       if (err) {
> -               err =3D -EIO;
> -               goto out;
> -       }
> -
>         i =3D 0;
>         while (bio_size > 0) {
>                 int page_size;
> @@ -1423,6 +1397,10 @@ static int ntfs_write_cb(struct ntfs_inode *ni, lo=
ff_t pos, struct page **pages,
>                 if (!bio) {
>                         bio =3D bio_alloc(vol->sb->s_bdev, 1, REQ_OP_WRIT=
E,
>                                         GFP_NOIO);
> +                       if (!bio) {
> +                               err =3D -ENOMEM;
> +                               goto free_rlc;
> +                       }
>                         bio->bi_iter.bi_sector =3D
>                                 ntfs_bytes_to_sector(vol,
>                                                 ntfs_cluster_to_bytes(vol=
, bio_lcn) +
> @@ -1433,7 +1411,7 @@ static int ntfs_write_cb(struct ntfs_inode *ni, lof=
f_t pos, struct page **pages,
>                         err =3D submit_bio_wait(bio);
>                         bio_put(bio);
>                         if (err)
> -                               goto out;
> +                               goto free_rlc;
>                         bio =3D NULL;
>                         goto setup_bio;
>                 }
> @@ -1442,6 +1420,37 @@ static int ntfs_write_cb(struct ntfs_inode *ni, lo=
ff_t pos, struct page **pages,
>
>         err =3D submit_bio_wait(bio);
>         bio_put(bio);
> +       if (err)
> +               goto free_rlc;
> +
> +       /* Do not discard the old compression block until the new one is =
safe. */
> +       err =3D ntfs_non_resident_attr_punch_hole(ni, new_vcn, cb_cluster=
s);
> +       if (err)
> +               goto free_rlc;
> +
> +       down_write(&ni->runlist.lock);
> +       rl =3D ntfs_runlists_merge(&ni->runlist, rlc, 0, &new_rl_count);
> +       if (IS_ERR(rl)) {
> +               up_write(&ni->runlist.lock);
> +               ntfs_error(vol->sb, "Failed to merge runlists");
> +               err =3D PTR_ERR(rl);
> +               goto free_rlc;
> +       }
> +
> +       ni->runlist.count =3D new_rl_count;
> +       ni->runlist.rl =3D rl;
> +       rlc =3D NULL;
> +
> +       err =3D ntfs_attr_update_mapping_pairs(ni, 0);
> +       up_write(&ni->runlist.lock);
> +       if (err)
> +               err =3D -EIO;
> +       goto out;
> +
> +free_rlc:
> +       if (ntfs_cluster_free_from_rl(vol, rlc))
> +               ntfs_error(vol->sb, "Failed to free hot clusters.");
> +       kvfree(rlc);
>  out:
>         if (outbuf)
>                 vunmap(outbuf);
> --
> 2.34.1
>


--=20
Thanks,
Hyunchul