From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
Same as udf_current_aext(), take pointer to etype to store the extent
type, while return 0 for success and <0 on error.
Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
fs/udf/balloc.c | 6 +++---
fs/udf/directory.c | 7 +++++--
fs/udf/inode.c | 50 +++++++++++++++++++++++-----------------------
fs/udf/super.c | 3 ++-
fs/udf/truncate.c | 8 ++++----
fs/udf/udfdecl.h | 5 +++--
6 files changed, 42 insertions(+), 37 deletions(-)
diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index d8fc11765d61..b216c43cf433 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -384,7 +384,7 @@ static void udf_table_free_blocks(struct super_block *sb,
epos.bh = oepos.bh = NULL;
while (count &&
- (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+ !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
if (((eloc.logicalBlockNum +
(elen >> sb->s_blocksize_bits)) == start)) {
if ((0x3FFFFFFF - elen) <
@@ -517,7 +517,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
eloc.logicalBlockNum = 0xFFFFFFFF;
while (first_block != eloc.logicalBlockNum &&
- (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+ !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
udf_debug("eloc=%u, elen=%u, first_block=%u\n",
eloc.logicalBlockNum, elen, first_block);
; /* empty loop body */
@@ -584,7 +584,7 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
epos.bh = goal_epos.bh = NULL;
while (spread &&
- (etype = udf_next_aext(table, &epos, &eloc, &elen, 1)) != -1) {
+ !udf_next_aext(table, &epos, &eloc, &elen, &etype, 1)) {
if (goal >= eloc.logicalBlockNum) {
if (goal < eloc.logicalBlockNum +
(elen >> sb->s_blocksize_bits))
diff --git a/fs/udf/directory.c b/fs/udf/directory.c
index 93153665eb37..f865538c985d 100644
--- a/fs/udf/directory.c
+++ b/fs/udf/directory.c
@@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
*/
static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
{
+ int8_t etype;
+ int err = 0;
iter->loffset++;
if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
return 0;
iter->loffset = 0;
- if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
- != (EXT_RECORDED_ALLOCATED >> 30)) {
+ err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
+ &etype, 1);
+ if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
if (iter->pos == iter->dir->i_size) {
iter->elen = 0;
return 0;
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 6d41ca0e7dba..ba980ce5e13a 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -545,6 +545,7 @@ static int udf_do_extend_file(struct inode *inode,
} else {
struct kernel_lb_addr tmploc;
uint32_t tmplen;
+ int8_t tmptype;
udf_write_aext(inode, last_pos, &last_ext->extLocation,
last_ext->extLength, 1);
@@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
* empty indirect extent.
*/
if (new_block_bytes)
- udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
+ udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
}
iinfo->i_lenExtents += add;
@@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
} else {
epos.offset -= adsize;
- etype = udf_next_aext(inode, &epos, &extent.extLocation,
- &extent.extLength, 0);
+ udf_next_aext(inode, &epos, &extent.extLocation,
+ &extent.extLength, &etype, 0);
extent.extLength |= etype << 30;
}
@@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
loff_t lbcount = 0, b_off = 0;
udf_pblk_t newblocknum;
sector_t offset = 0;
- int8_t etype;
+ int8_t etype, tmpetype;
struct udf_inode_info *iinfo = UDF_I(inode);
udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
int lastblock = 0;
@@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
prev_epos.offset = cur_epos.offset;
cur_epos.offset = next_epos.offset;
- etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
- if (etype == -1)
+ ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
+ if (ret)
break;
c = !c;
@@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
* Move prev_epos and cur_epos into indirect extent if we are at
* the pointer to it
*/
- udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
- udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
+ udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
+ udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
/* if the extent is allocated and recorded, return the block
if the extent is not a multiple of the blocksize, round up */
@@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
}
/* Are we beyond EOF and preallocated extent? */
- if (etype == -1) {
+ if (ret < 0) {
loff_t hole_len;
isBeyondEOF = true;
@@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
/* if the current block is located in an extent,
read the next extent */
- etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
- if (etype != -1) {
+ if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
laarr[c + 1].extLength = (etype << 30) | elen;
laarr[c + 1].extLocation = eloc;
count++;
@@ -1172,6 +1172,7 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
int start = 0, i;
struct kernel_lb_addr tmploc;
uint32_t tmplen;
+ int8_t tmptype;
int err;
if (startnum > endnum) {
@@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
if (err < 0)
return err;
udf_next_aext(inode, epos, &laarr[i].extLocation,
- &laarr[i].extLength, 1);
+ &laarr[i].extLength, &tmptype, 1);
start++;
}
}
for (i = start; i < endnum; i++) {
- udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
+ udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
udf_write_aext(inode, epos, &laarr[i].extLocation,
laarr[i].extLength, 1);
}
@@ -2168,15 +2169,15 @@ void udf_write_aext(struct inode *inode, struct extent_position *epos,
*/
#define UDF_MAX_INDIR_EXTS 16
-int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
- struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
+int udf_next_aext(struct inode *inode, struct extent_position *epos,
+ struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
+ int inc)
{
- int8_t etype;
unsigned int indirections = 0;
int err = 0;
- while ((err = udf_current_aext(inode, epos, eloc, elen, &etype, inc))) {
- if (err || etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
+ while ((err = udf_current_aext(inode, epos, eloc, elen, etype, inc))) {
+ if (err || *etype != (EXT_NEXT_EXTENT_ALLOCDESCS >> 30))
break;
udf_pblk_t block;
@@ -2185,7 +2186,7 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
udf_err(inode->i_sb,
"too many indirect extents in inode %lu\n",
inode->i_ino);
- return -1;
+ return -EFSCORRUPTED;
}
epos->block = *eloc;
@@ -2195,7 +2196,7 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
epos->bh = sb_bread(inode->i_sb, block);
if (!epos->bh) {
udf_debug("reading block %u failed!\n", block);
- return -1;
+ return -EIO;
}
}
@@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
if (epos.bh)
get_bh(epos.bh);
- while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
+ while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
udf_write_aext(inode, &epos, &neloc, nelen, 1);
neloc = oeloc;
nelen = (etype << 30) | oelen;
@@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
adsize = 0;
oepos = epos;
- if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
+ if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
return -1;
- while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
+ while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
if (oepos.bh != epos.bh) {
oepos.block = epos.block;
@@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
}
*elen = 0;
do {
- etype = udf_next_aext(inode, pos, eloc, elen, 1);
- if (etype == -1) {
+ if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
*offset = (bcount - lbcount) >> blocksize_bits;
iinfo->i_lenExtents = lbcount;
return -1;
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 3460ecc826d1..8c34224e1aee 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -2482,13 +2482,14 @@ static unsigned int udf_count_free_table(struct super_block *sb,
uint32_t elen;
struct kernel_lb_addr eloc;
struct extent_position epos;
+ int8_t etype;
mutex_lock(&UDF_SB(sb)->s_alloc_mutex);
epos.block = UDF_I(table)->i_location;
epos.offset = sizeof(struct unallocSpaceEntry);
epos.bh = NULL;
- while (udf_next_aext(table, &epos, &eloc, &elen, 1) != -1)
+ while (!udf_next_aext(table, &epos, &eloc, &elen, &etype, 1))
accum += (elen >> table->i_sb->s_blocksize_bits);
brelse(epos.bh);
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index 91b6e2698e7e..b7361222f988 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
BUG();
/* Find the last extent in the file */
- while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
+ while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
etype = netype;
lbcount += elen;
if (lbcount > inode->i_size) {
@@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
epos.offset -= adsize;
extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
epos.offset += adsize;
- if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
+ if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
udf_err(inode->i_sb,
"Extent after EOF in inode %u\n",
(unsigned)inode->i_ino);
@@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
epos.block = iinfo->i_location;
/* Find the last extent in the file */
- while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
+ while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
brelse(prev_epos.bh);
prev_epos = epos;
if (prev_epos.bh)
get_bh(prev_epos.bh);
- etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
+ udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
lbcount += elen;
}
if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index d893db95ac70..5067ed68a8b4 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -169,8 +169,9 @@ extern int udf_add_aext(struct inode *, struct extent_position *,
extern void udf_write_aext(struct inode *, struct extent_position *,
struct kernel_lb_addr *, uint32_t, int);
extern int8_t udf_delete_aext(struct inode *, struct extent_position);
-extern int8_t udf_next_aext(struct inode *, struct extent_position *,
- struct kernel_lb_addr *, uint32_t *, int);
+extern int udf_next_aext(struct inode *inode, struct extent_position *epos,
+ struct kernel_lb_addr *eloc, uint32_t *elen,
+ int8_t *etype, int inc);
extern int udf_current_aext(struct inode *inode, struct extent_position *epos,
struct kernel_lb_addr *eloc, uint32_t *elen,
int8_t *etype, int inc);
--
2.43.0On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>
> Same as udf_current_aext(), take pointer to etype to store the extent
> type, while return 0 for success and <0 on error.
>
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
...
> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
> index 93153665eb37..f865538c985d 100644
> --- a/fs/udf/directory.c
> +++ b/fs/udf/directory.c
> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
> */
> static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
> {
> + int8_t etype;
> + int err = 0;
Nit: please add empty line between declaration and the code.
> iter->loffset++;
> if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
> return 0;
>
> iter->loffset = 0;
> - if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
> - != (EXT_RECORDED_ALLOCATED >> 30)) {
> + err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
> + &etype, 1);
> + if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
> if (iter->pos == iter->dir->i_size) {
> iter->elen = 0;
> return 0;
...
> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
> * empty indirect extent.
> */
> if (new_block_bytes)
> - udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
> + udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
> }
> iinfo->i_lenExtents += add;
>
Hum, this will need error checking but we can leave that for future
patches.
> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
> extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
> } else {
> epos.offset -= adsize;
> - etype = udf_next_aext(inode, &epos, &extent.extLocation,
> - &extent.extLength, 0);
> + udf_next_aext(inode, &epos, &extent.extLocation,
> + &extent.extLength, &etype, 0);
> extent.extLength |= etype << 30;
> }
>
> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
> loff_t lbcount = 0, b_off = 0;
> udf_pblk_t newblocknum;
> sector_t offset = 0;
> - int8_t etype;
> + int8_t etype, tmpetype;
> struct udf_inode_info *iinfo = UDF_I(inode);
> udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
> int lastblock = 0;
> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
> prev_epos.offset = cur_epos.offset;
> cur_epos.offset = next_epos.offset;
>
> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
> - if (etype == -1)
> + ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
> + if (ret)
> break;
I think here we need to add error handling as well and we should probably
do it in this patch / patch series. If ret is ENODATA, we just break out
from the cycle but if ret is some other error, we need to return that error
from inode_getblk().
> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
> * Move prev_epos and cur_epos into indirect extent if we are at
> * the pointer to it
> */
> - udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
> - udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
> + udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
> + udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
Again, this should have error handling now.
>
> /* if the extent is allocated and recorded, return the block
> if the extent is not a multiple of the blocksize, round up */
> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
> }
>
> /* Are we beyond EOF and preallocated extent? */
> - if (etype == -1) {
> + if (ret < 0) {
I'd prefer ret == -ENODATA to make this explicit.
> loff_t hole_len;
>
> isBeyondEOF = true;
> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>
> /* if the current block is located in an extent,
> read the next extent */
> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
> - if (etype != -1) {
> + if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
> laarr[c + 1].extLength = (etype << 30) | elen;
> laarr[c + 1].extLocation = eloc;
> count++;
And this should be distinguisting between EOF and other errors so that we
don't set lastblock wrongly. Instead we should bail with error.
> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
> if (err < 0)
> return err;
> udf_next_aext(inode, epos, &laarr[i].extLocation,
> - &laarr[i].extLength, 1);
> + &laarr[i].extLength, &tmptype, 1);
> start++;
> }
> }
>
> for (i = start; i < endnum; i++) {
> - udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
> + udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
> udf_write_aext(inode, epos, &laarr[i].extLocation,
> laarr[i].extLength, 1);
> }
Again these two calls should have error handling now. udf_update_extents()
is already able to return errors.
> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
> if (epos.bh)
> get_bh(epos.bh);
>
> - while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
> + while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
> udf_write_aext(inode, &epos, &neloc, nelen, 1);
> neloc = oeloc;
> nelen = (etype << 30) | oelen;
Here, we should check if udf_next_aext() returned error (other than
ENODATA) and bail in that case instead of trying to insert new extent.
> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
> adsize = 0;
>
> oepos = epos;
> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
> + if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
> return -1;
>
> - while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
> udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
> if (oepos.bh != epos.bh) {
> oepos.block = epos.block;
> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
> }
> *elen = 0;
> do {
> - etype = udf_next_aext(inode, pos, eloc, elen, 1);
> - if (etype == -1) {
> + if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
> *offset = (bcount - lbcount) >> blocksize_bits;
> iinfo->i_lenExtents = lbcount;
> return -1;
Again, here we need to distinguish ENODATA from other errors so that we
don't wrongly consider failure to read extent like EOF.
> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> index 91b6e2698e7e..b7361222f988 100644
> --- a/fs/udf/truncate.c
> +++ b/fs/udf/truncate.c
> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
> BUG();
>
> /* Find the last extent in the file */
> - while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
> etype = netype;
> lbcount += elen;
> if (lbcount > inode->i_size) {
This should be checking for error (after the loop) so that we don't
accidentally try to truncate extents early in case of error.
> @@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
> epos.offset -= adsize;
> extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
> epos.offset += adsize;
> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
> + if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
> udf_err(inode->i_sb,
> "Extent after EOF in inode %u\n",
> (unsigned)inode->i_ino);
> @@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
> epos.block = iinfo->i_location;
>
> /* Find the last extent in the file */
> - while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
> brelse(prev_epos.bh);
> prev_epos = epos;
> if (prev_epos.bh)
> get_bh(prev_epos.bh);
>
> - etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
> + udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
> lbcount += elen;
> }
> if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
Again error checking for above two calls plus here we should not depend on
'etype' value after udf_next_aext() returned error. So we'll need another
temporary variable for etype to pass to the first udf_next_aext() call.
Thanks!
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
On 2024/9/20 23:47, Jan Kara wrote:
> On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> Same as udf_current_aext(), take pointer to etype to store the extent
>> type, while return 0 for success and <0 on error.
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>
> ...
>
>> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
>> index 93153665eb37..f865538c985d 100644
>> --- a/fs/udf/directory.c
>> +++ b/fs/udf/directory.c
>> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>> */
>> static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>> {
>> + int8_t etype;
>> + int err = 0;
>
> Nit: please add empty line between declaration and the code.
>
>> iter->loffset++;
>> if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>> return 0;
>>
>> iter->loffset = 0;
>> - if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
>> - != (EXT_RECORDED_ALLOCATED >> 30)) {
>> + err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
>> + &etype, 1);
>> + if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>> if (iter->pos == iter->dir->i_size) {
>> iter->elen = 0;
>> return 0;
>
> ...
>
>> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>> * empty indirect extent.
>> */
>> if (new_block_bytes)
>> - udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>> }
>> iinfo->i_lenExtents += add;
>>
>
> Hum, this will need error checking but we can leave that for future
> patches.
>
>> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>> extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>> } else {
>> epos.offset -= adsize;
>> - etype = udf_next_aext(inode, &epos, &extent.extLocation,
>> - &extent.extLength, 0);
>> + udf_next_aext(inode, &epos, &extent.extLocation,
>> + &extent.extLength, &etype, 0);
>> extent.extLength |= etype << 30;
>> }
>>
>> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> loff_t lbcount = 0, b_off = 0;
>> udf_pblk_t newblocknum;
>> sector_t offset = 0;
>> - int8_t etype;
>> + int8_t etype, tmpetype;
>> struct udf_inode_info *iinfo = UDF_I(inode);
>> udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>> int lastblock = 0;
>> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> prev_epos.offset = cur_epos.offset;
>> cur_epos.offset = next_epos.offset;
>>
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
>> - if (etype == -1)
>> + ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
>> + if (ret)
>> break;
>
> I think here we need to add error handling as well and we should probably
> do it in this patch / patch series. If ret is ENODATA, we just break out
> from the cycle but if ret is some other error, we need to return that error
> from inode_getblk().
>
>> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> * Move prev_epos and cur_epos into indirect extent if we are at
>> * the pointer to it
>> */
>> - udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
>> - udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
>> + udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>> + udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>
> Again, this should have error handling now.
>
>>
>> /* if the extent is allocated and recorded, return the block
>> if the extent is not a multiple of the blocksize, round up */
>> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> }
>>
>> /* Are we beyond EOF and preallocated extent? */
>> - if (etype == -1) {
>> + if (ret < 0) {
>
> I'd prefer ret == -ENODATA to make this explicit.
>
>> loff_t hole_len;
>>
>> isBeyondEOF = true;
>> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>
>> /* if the current block is located in an extent,
>> read the next extent */
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
>> - if (etype != -1) {
>> + if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>> laarr[c + 1].extLength = (etype << 30) | elen;
>> laarr[c + 1].extLocation = eloc;
>> count++;
>
> And this should be distinguisting between EOF and other errors so that we
> don't set lastblock wrongly. Instead we should bail with error.
>
>> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>> if (err < 0)
>> return err;
>> udf_next_aext(inode, epos, &laarr[i].extLocation,
>> - &laarr[i].extLength, 1);
>> + &laarr[i].extLength, &tmptype, 1);
>> start++;
>> }
>> }
>>
>> for (i = start; i < endnum; i++) {
>> - udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>> udf_write_aext(inode, epos, &laarr[i].extLocation,
>> laarr[i].extLength, 1);
>> }
>
> Again these two calls should have error handling now. udf_update_extents()
> is already able to return errors.
>
>> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>> if (epos.bh)
>> get_bh(epos.bh);
>>
>> - while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>> udf_write_aext(inode, &epos, &neloc, nelen, 1);
>> neloc = oeloc;
>> nelen = (etype << 30) | oelen;
>
> Here, we should check if udf_next_aext() returned error (other than
> ENODATA) and bail in that case instead of trying to insert new extent.
>
>> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>> adsize = 0;
>>
>> oepos = epos;
>> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
>> + if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>> return -1;
>>
>> - while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>> udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>> if (oepos.bh != epos.bh) {
>> oepos.block = epos.block;
>> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>> }
>> *elen = 0;
>> do {
>> - etype = udf_next_aext(inode, pos, eloc, elen, 1);
>> - if (etype == -1) {
>> + if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>> *offset = (bcount - lbcount) >> blocksize_bits;
>> iinfo->i_lenExtents = lbcount;
>> return -1;
>
> Again, here we need to distinguish ENODATA from other errors so that we
> don't wrongly consider failure to read extent like EOF.
>
>> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
>> index 91b6e2698e7e..b7361222f988 100644
>> --- a/fs/udf/truncate.c
>> +++ b/fs/udf/truncate.c
>> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>> BUG();
>>
>> /* Find the last extent in the file */
>> - while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>> etype = netype;
>> lbcount += elen;
>> if (lbcount > inode->i_size) {
>
> This should be checking for error (after the loop) so that we don't
> accidentally try to truncate extents early in case of error.
>
Sorry to bother, in case of error(including EOF), it won't go into the loop and
has chance to call extent_trunc(). After the loop, only some update and clean op,
iinfo->i_lenExtents = inode->i_size;
brelse(epos.bh);
So I'm a little confused which part of this piece of code needs to change?
On Tue 24-09-24 20:08:56, Zhao Mengmeng wrote:
> On 2024/9/20 23:47, Jan Kara wrote:
> > On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
> >> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
> >> index 91b6e2698e7e..b7361222f988 100644
> >> --- a/fs/udf/truncate.c
> >> +++ b/fs/udf/truncate.c
> >> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
> >> BUG();
> >>
> >> /* Find the last extent in the file */
> >> - while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
> >> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
> >> etype = netype;
> >> lbcount += elen;
> >> if (lbcount > inode->i_size) {
> >
> > This should be checking for error (after the loop) so that we don't
> > accidentally try to truncate extents early in case of error.
> >
> Sorry to bother, in case of error(including EOF), it won't go into the loop and
> has chance to call extent_trunc(). After the loop, only some update and clean op,
>
> iinfo->i_lenExtents = inode->i_size;
> brelse(epos.bh);
>
> So I'm a little confused which part of this piece of code needs to change?
So if we are not able to scan until EOF due to error, we should set
i_lenExtents to i_size but you're right this is mostly a cosmetic thing.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
On 2024/9/20 23:47, Jan Kara wrote:
> On Wed 18-09-24 17:36:33, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> Same as udf_current_aext(), take pointer to etype to store the extent
>> type, while return 0 for success and <0 on error.
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>
> ...
>
>> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
>> index 93153665eb37..f865538c985d 100644
>> --- a/fs/udf/directory.c
>> +++ b/fs/udf/directory.c
>> @@ -166,13 +166,16 @@ static struct buffer_head *udf_fiiter_bread_blk(struct udf_fileident_iter *iter)
>> */
>> static int udf_fiiter_advance_blk(struct udf_fileident_iter *iter)
>> {
>> + int8_t etype;
>> + int err = 0;
>
> Nit: please add empty line between declaration and the code.
Got it.
>> iter->loffset++;
>> if (iter->loffset < DIV_ROUND_UP(iter->elen, 1<<iter->dir->i_blkbits))
>> return 0;
>>
>> iter->loffset = 0;
>> - if (udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen, 1)
>> - != (EXT_RECORDED_ALLOCATED >> 30)) {
>> + err = udf_next_aext(iter->dir, &iter->epos, &iter->eloc, &iter->elen,
>> + &etype, 1);
>> + if (err || etype != (EXT_RECORDED_ALLOCATED >> 30)) {
>> if (iter->pos == iter->dir->i_size) {
>> iter->elen = 0;
>> return 0;
>
> ...
>
>> @@ -555,7 +556,7 @@ static int udf_do_extend_file(struct inode *inode,
>> * empty indirect extent.
>> */
>> if (new_block_bytes)
>> - udf_next_aext(inode, last_pos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, last_pos, &tmploc, &tmplen, &tmptype, 0);
>> }
>> iinfo->i_lenExtents += add;
>>
>
> Hum, this will need error checking but we can leave that for future
> patches.
Yes, will add in this series.
>> @@ -674,8 +675,8 @@ static int udf_extend_file(struct inode *inode, loff_t newsize)
>> extent.extLength = EXT_NOT_RECORDED_NOT_ALLOCATED;
>> } else {
>> epos.offset -= adsize;
>> - etype = udf_next_aext(inode, &epos, &extent.extLocation,
>> - &extent.extLength, 0);
>> + udf_next_aext(inode, &epos, &extent.extLocation,
>> + &extent.extLength, &etype, 0);
>> extent.extLength |= etype << 30;
>> }
>>
>> @@ -712,7 +713,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> loff_t lbcount = 0, b_off = 0;
>> udf_pblk_t newblocknum;
>> sector_t offset = 0;
>> - int8_t etype;
>> + int8_t etype, tmpetype;
>> struct udf_inode_info *iinfo = UDF_I(inode);
>> udf_pblk_t goal = 0, pgoal = iinfo->i_location.logicalBlockNum;
>> int lastblock = 0;
>> @@ -748,8 +749,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> prev_epos.offset = cur_epos.offset;
>> cur_epos.offset = next_epos.offset;
>>
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 1);
>> - if (etype == -1)
>> + ret = udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 1);
>> + if (ret)
>> break;
>
> I think here we need to add error handling as well and we should probably
> do it in this patch / patch series. If ret is ENODATA, we just break out
> from the cycle but if ret is some other error, we need to return that error
> from inode_getblk().
>
>> @@ -771,8 +772,8 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> * Move prev_epos and cur_epos into indirect extent if we are at
>> * the pointer to it
>> */
>> - udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, 0);
>> - udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, 0);
>> + udf_next_aext(inode, &prev_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>> + udf_next_aext(inode, &cur_epos, &tmpeloc, &tmpelen, &tmpetype, 0);
>
> Again, this should have error handling now.
>
>>
>> /* if the extent is allocated and recorded, return the block
>> if the extent is not a multiple of the blocksize, round up */
>> @@ -793,7 +794,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>> }
>>
>> /* Are we beyond EOF and preallocated extent? */
>> - if (etype == -1) {
>> + if (ret < 0) {
>
> I'd prefer ret == -ENODATA to make this explicit.
>
>> loff_t hole_len;
>>
>> isBeyondEOF = true;
>> @@ -846,8 +847,7 @@ static int inode_getblk(struct inode *inode, struct udf_map_rq *map)
>>
>> /* if the current block is located in an extent,
>> read the next extent */
>> - etype = udf_next_aext(inode, &next_epos, &eloc, &elen, 0);
>> - if (etype != -1) {
>> + if (!udf_next_aext(inode, &next_epos, &eloc, &elen, &etype, 0)) {
>> laarr[c + 1].extLength = (etype << 30) | elen;
>> laarr[c + 1].extLocation = eloc;
>> count++;
>
> And this should be distinguisting between EOF and other errors so that we
> don't set lastblock wrongly. Instead we should bail with error.
>
>> @@ -1190,13 +1191,13 @@ static int udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr,
>> if (err < 0)
>> return err;
>> udf_next_aext(inode, epos, &laarr[i].extLocation,
>> - &laarr[i].extLength, 1);
>> + &laarr[i].extLength, &tmptype, 1);
>> start++;
>> }
>> }
>>
>> for (i = start; i < endnum; i++) {
>> - udf_next_aext(inode, epos, &tmploc, &tmplen, 0);
>> + udf_next_aext(inode, epos, &tmploc, &tmplen, &tmptype, 0);
>> udf_write_aext(inode, epos, &laarr[i].extLocation,
>> laarr[i].extLength, 1);
>> }
>
> Again these two calls should have error handling now. udf_update_extents()
> is already able to return errors.
>
>> @@ -2267,7 +2268,7 @@ static int udf_insert_aext(struct inode *inode, struct extent_position epos,
>> if (epos.bh)
>> get_bh(epos.bh);
>>
>> - while ((etype = udf_next_aext(inode, &epos, &oeloc, &oelen, 0)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &oeloc, &oelen, &etype, 0)) {
>> udf_write_aext(inode, &epos, &neloc, nelen, 1);
>> neloc = oeloc;
>> nelen = (etype << 30) | oelen;
>
> Here, we should check if udf_next_aext() returned error (other than
> ENODATA) and bail in that case instead of trying to insert new extent.
>
>> @@ -2302,10 +2303,10 @@ int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
>> adsize = 0;
>>
>> oepos = epos;
>> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) == -1)
>> + if (udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1))
>> return -1;
>>
>> - while ((etype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1)) {
>> udf_write_aext(inode, &oepos, &eloc, (etype << 30) | elen, 1);
>> if (oepos.bh != epos.bh) {
>> oepos.block = epos.block;
>> @@ -2379,8 +2380,7 @@ int8_t inode_bmap(struct inode *inode, sector_t block,
>> }
>> *elen = 0;
>> do {
>> - etype = udf_next_aext(inode, pos, eloc, elen, 1);
>> - if (etype == -1) {
>> + if (udf_next_aext(inode, pos, eloc, elen, &etype, 1)) {
>> *offset = (bcount - lbcount) >> blocksize_bits;
>> iinfo->i_lenExtents = lbcount;
>> return -1;
>
> Again, here we need to distinguish ENODATA from other errors so that we
> don't wrongly consider failure to read extent like EOF.
>
>> diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
>> index 91b6e2698e7e..b7361222f988 100644
>> --- a/fs/udf/truncate.c
>> +++ b/fs/udf/truncate.c
>> @@ -85,7 +85,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>> BUG();
>>
>> /* Find the last extent in the file */
>> - while ((netype = udf_next_aext(inode, &epos, &eloc, &elen, 1)) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1)) {
>> etype = netype;
>> lbcount += elen;
>> if (lbcount > inode->i_size) {
>
> This should be checking for error (after the loop) so that we don't
> accidentally try to truncate extents early in case of error.
>
>> @@ -101,7 +101,7 @@ void udf_truncate_tail_extent(struct inode *inode)
>> epos.offset -= adsize;
>> extent_trunc(inode, &epos, &eloc, etype, elen, nelen);
>> epos.offset += adsize;
>> - if (udf_next_aext(inode, &epos, &eloc, &elen, 1) != -1)
>> + if (!udf_next_aext(inode, &epos, &eloc, &elen, &netype, 1))
>> udf_err(inode->i_sb,
>> "Extent after EOF in inode %u\n",
>> (unsigned)inode->i_ino);
>> @@ -132,13 +132,13 @@ void udf_discard_prealloc(struct inode *inode)
>> epos.block = iinfo->i_location;
>>
>> /* Find the last extent in the file */
>> - while (udf_next_aext(inode, &epos, &eloc, &elen, 0) != -1) {
>> + while (!udf_next_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
>> brelse(prev_epos.bh);
>> prev_epos = epos;
>> if (prev_epos.bh)
>> get_bh(prev_epos.bh);
>>
>> - etype = udf_next_aext(inode, &epos, &eloc, &elen, 1);
>> + udf_next_aext(inode, &epos, &eloc, &elen, &etype, 1);
>> lbcount += elen;
>> }
>> if (etype == (EXT_NOT_RECORDED_ALLOCATED >> 30)) {
>
> Again error checking for above two calls plus here we should not depend on
> 'etype' value after udf_next_aext() returned error. So we'll need another
> temporary variable for etype to pass to the first udf_next_aext() call.
>
> Thanks!
>
> Honza
Will fix them in V2. Thanks.
© 2016 - 2024 Red Hat, Inc.