On Sat 09-01-21 16:40:53, Steve Magnani wrote:
> From: Steven J. Magnani <magn...@ieee.org>
>
> When an ALLOCATED_NOT_RECORDED extent ("File Tail") follows the
> extents describing a file body, don't (improperly) try to make use of it
> as part of appending a hole to the file.
>
> Fixes: fa33cdbf3ece ("udf: Fix incorrect final NOT_ALLOCATED (hole) extent
> length")
> Signed-off-by: Steven J. Magnani <magn...@ieee.org>
Thanks for the fix! One comment below.
> --- a/fs/udf/inode.c 2020-12-28 20:51:29.000000000 -0600
> +++ b/fs/udf/inode.c 2021-01-02 17:00:39.794157840 -0600
> @@ -520,8 +520,7 @@ static int udf_do_extend_file(struct ino
> prealloc_loc = last_ext->extLocation;
> prealloc_len = last_ext->extLength;
> /* Mark the extent as a hole */
> - last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED |
> - (last_ext->extLength & UDF_EXTENT_LENGTH_MASK);
> + last_ext->extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
> last_ext->extLocation.logicalBlockNum = 0;
> last_ext->extLocation.partitionReferenceNum = 0;
> }
> @@ -626,7 +625,6 @@ static void udf_do_extend_final_block(st
>
> static int udf_extend_file(struct inode *inode, loff_t newsize)
> {
> -
> struct extent_position epos;
> struct kernel_lb_addr eloc;
> uint32_t elen;
> @@ -639,6 +637,7 @@ static int udf_extend_file(struct inode
> struct kernel_long_ad extent;
> int err = 0;
> int within_final_block;
> + loff_t hole_size = 0;
>
> if (iinfo->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
> adsize = sizeof(struct short_ad);
> @@ -648,7 +647,8 @@ static int udf_extend_file(struct inode
> BUG();
>
> etype = inode_bmap(inode, first_block, &epos, &eloc, &elen, &offset);
> - within_final_block = (etype != -1);
> + within_final_block = (etype == (EXT_RECORDED_ALLOCATED >> 30)) ||
> + (etype == (EXT_NOT_RECORDED_NOT_ALLOCATED >> 30));
>
> if ((!epos.bh && epos.offset == udf_file_entry_alloc_offset(inode)) ||
> (epos.bh && epos.offset == sizeof(struct allocExtDesc))) {
> @@ -658,9 +658,15 @@ static int udf_extend_file(struct inode
> extent.extLocation.partitionReferenceNum = 0;
> extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
> } else {
> + int8_t bmap_etype = etype;
> epos.offset -= adsize;
> etype = udf_next_aext(inode, &epos, &extent.extLocation,
> &extent.extLength, 0);
> + if ((bmap_etype == -1) &&
> + (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30))) {
> + /* offset is relative to prealloc extent end */
> + hole_size = extent.extLength;
Rather than introducing 'hole_size', I think we can just update 'offset'
here?
> + }
> extent.extLength |= etype << 30;
> }
>
...
> @@ -729,14 +735,22 @@ static sector_t inode_getblk(struct inod
> cur_epos.bh = next_epos.bh;
> }
>
> - lbcount += elen;
> -
> prev_epos.block = cur_epos.block;
> cur_epos.block = next_epos.block;
>
> prev_epos.offset = cur_epos.offset;
> cur_epos.offset = next_epos.offset;
>
> + /* Corner case: preallocated extent that stops short of
> + * desired block
> + */
> + if ((etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) &&
> + ((lbcount + elen) <= b_off)) {
> + etype = -1;
> + break;
> + }
> +
> + lbcount += elen;
Honestly, I don't like this special case and it seems rather arbitrary.
Also any EXT_NOT_RECORDED_ALLOCATED inside the file would now break the
block lookup while previously it was gracefully handled. Also I'm not sure
why it's event needed - even if inode_getblk() returns returns -1 and the
'extent' loaded by udf_next_aext() ends up being EXT_NOT_RECORDED_ALLOCATED,
we will end up passing it to udf_do_extend_file() which recognizes it as
preallocation extent and will insert a hole extent before it? Am I missing
something?
Honza
> etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
> if (etype == -1)
> break;
>
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
