Messages in this thread | | | Date | Thu, 14 Jan 2021 18:02:12 +0100 | From | Jan Kara <> | Subject | Re: [PATCH 1/2] udf: fix hole append when File Tail exists |
| |
On Sat 09-01-21 16:40:53, Steve Magnani wrote: > From: Steven J. Magnani <magnani@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 <magnani@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 <jack@suse.com> SUSE Labs, CR
| |