From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
As Jan suggested in links below, refactor udf_current_aext() to
differentiate between error and "hit EOF", it now takes pointer to etype
to store the extent type, return 0 when get etype success; return -ENODATA
when hit EOF; return -EINVAL when i_alloc_type invalid.
Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
---
fs/udf/inode.c | 37 +++++++++++++++++++++++--------------
fs/udf/truncate.c | 3 +--
fs/udf/udfdecl.h | 5 +++--
3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index 4726a4d014b6..6d41ca0e7dba 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1955,6 +1955,7 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
struct extent_position nepos;
struct kernel_lb_addr neloc;
int ver, adsize;
+ int err = 0;
if (UDF_I(inode)->i_alloc_type == ICBTAG_FLAG_AD_SHORT)
adsize = sizeof(struct short_ad);
@@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
if (epos->offset + adsize > sb->s_blocksize) {
struct kernel_lb_addr cp_loc;
uint32_t cp_len;
- int cp_type;
+ int8_t cp_type;
epos->offset -= adsize;
- cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
+ err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
+ if (err < 0)
+ goto err_out;
cp_len |= ((uint32_t)cp_type) << 30;
__udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
@@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
*epos = nepos;
return 0;
+err_out:
+ brelse(epos->bh);
+ return err;
}
/*
@@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
{
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))
+ break;
- while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
- (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
udf_pblk_t block;
if (++indirections > UDF_MAX_INDIR_EXTS) {
@@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
}
}
- return etype;
+ return err;
}
-int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
- struct kernel_lb_addr *eloc, uint32_t *elen, int inc)
+int udf_current_aext(struct inode *inode, struct extent_position *epos,
+ struct kernel_lb_addr *eloc, uint32_t *elen, int8_t *etype,
+ int inc)
{
int alen;
- int8_t etype;
uint8_t *ptr;
struct short_ad *sad;
struct long_ad *lad;
@@ -2224,8 +2233,8 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
case ICBTAG_FLAG_AD_SHORT:
sad = udf_get_fileshortad(ptr, alen, &epos->offset, inc);
if (!sad)
- return -1;
- etype = le32_to_cpu(sad->extLength) >> 30;
+ return -ENODATA;
+ *etype = le32_to_cpu(sad->extLength) >> 30;
eloc->logicalBlockNum = le32_to_cpu(sad->extPosition);
eloc->partitionReferenceNum =
iinfo->i_location.partitionReferenceNum;
@@ -2234,17 +2243,17 @@ int8_t udf_current_aext(struct inode *inode, struct extent_position *epos,
case ICBTAG_FLAG_AD_LONG:
lad = udf_get_filelongad(ptr, alen, &epos->offset, inc);
if (!lad)
- return -1;
- etype = le32_to_cpu(lad->extLength) >> 30;
+ return -ENODATA;
+ *etype = le32_to_cpu(lad->extLength) >> 30;
*eloc = lelb_to_cpu(lad->extLocation);
*elen = le32_to_cpu(lad->extLength) & UDF_EXTENT_LENGTH_MASK;
break;
default:
udf_debug("alloc_type = %u unsupported\n", iinfo->i_alloc_type);
- return -1;
+ return -EINVAL;
}
- return etype;
+ return 0;
}
static int udf_insert_aext(struct inode *inode, struct extent_position epos,
diff --git a/fs/udf/truncate.c b/fs/udf/truncate.c
index a686c10fd709..91b6e2698e7e 100644
--- a/fs/udf/truncate.c
+++ b/fs/udf/truncate.c
@@ -217,8 +217,7 @@ int udf_truncate_extents(struct inode *inode)
else
lenalloc -= sizeof(struct allocExtDesc);
- while ((etype = udf_current_aext(inode, &epos, &eloc,
- &elen, 0)) != -1) {
+ while (!udf_current_aext(inode, &epos, &eloc, &elen, &etype, 0)) {
if (etype == (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
udf_write_aext(inode, &epos, &neloc, nelen, 0);
if (indirect_ext_len) {
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 88692512a466..d893db95ac70 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -171,8 +171,9 @@ extern void udf_write_aext(struct inode *, struct extent_position *,
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 int8_t udf_current_aext(struct inode *, struct extent_position *,
- struct kernel_lb_addr *, uint32_t *, int);
+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);
extern void udf_update_extra_perms(struct inode *inode, umode_t mode);
/* misc.c */
--
2.43.0On Wed 18-09-24 17:36:32, Zhao Mengmeng wrote:
> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>
> As Jan suggested in links below, refactor udf_current_aext() to
> differentiate between error and "hit EOF", it now takes pointer to etype
> to store the extent type, return 0 when get etype success; return -ENODATA
> when hit EOF; return -EINVAL when i_alloc_type invalid.
>
> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
>
> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
Two comments below.
> @@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
> if (epos->offset + adsize > sb->s_blocksize) {
> struct kernel_lb_addr cp_loc;
> uint32_t cp_len;
> - int cp_type;
> + int8_t cp_type;
>
> epos->offset -= adsize;
> - cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
> + err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
> + if (err < 0)
> + goto err_out;
> cp_len |= ((uint32_t)cp_type) << 30;
>
> __udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
> @@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
> *epos = nepos;
>
> return 0;
> +err_out:
> + brelse(epos->bh);
> + return err;
> }
So here I don't think we want to release epos->bh. Rather we need to
release 'bh' itself which we have got because we did't replace epos->bh yet
with it.
> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
> {
> 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))
> + break;
>
> - while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
> - (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
> udf_pblk_t block;
>
> if (++indirections > UDF_MAX_INDIR_EXTS) {
> @@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
> }
> }
>
> - return etype;
> + return err;
> }
This doesn't look right. Probably it gets fixed up in the following patches
but here should be something like: !err ? etype : -1
to keep udf_next_aext() compatible with its users.
Otherwise the patch looks good.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
On 2024/9/20 23:09, Jan Kara wrote:
> On Wed 18-09-24 17:36:32, Zhao Mengmeng wrote:
>> From: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>>
>> As Jan suggested in links below, refactor udf_current_aext() to
>> differentiate between error and "hit EOF", it now takes pointer to etype
>> to store the extent type, return 0 when get etype success; return -ENODATA
>> when hit EOF; return -EINVAL when i_alloc_type invalid.
>>
>> Link: https://lore.kernel.org/all/20240912111235.6nr3wuqvktecy3vh@quack3/
>>
>> Signed-off-by: Zhao Mengmeng <zhaomengmeng@kylinos.cn>
>
> Two comments below.
>
>> @@ -1999,10 +2000,12 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>> if (epos->offset + adsize > sb->s_blocksize) {
>> struct kernel_lb_addr cp_loc;
>> uint32_t cp_len;
>> - int cp_type;
>> + int8_t cp_type;
>>
>> epos->offset -= adsize;
>> - cp_type = udf_current_aext(inode, epos, &cp_loc, &cp_len, 0);
>> + err = udf_current_aext(inode, epos, &cp_loc, &cp_len, &cp_type, 0);
>> + if (err < 0)
>> + goto err_out;
>> cp_len |= ((uint32_t)cp_type) << 30;
>>
>> __udf_add_aext(inode, &nepos, &cp_loc, cp_len, 1);
>> @@ -2017,6 +2020,9 @@ int udf_setup_indirect_aext(struct inode *inode, udf_pblk_t block,
>> *epos = nepos;
>>
>> return 0;
>> +err_out:
>> + brelse(epos->bh);
>> + return err;
>> }
>
> So here I don't think we want to release epos->bh. Rather we need to
> release 'bh' itself which we have got because we did't replace epos->bh yet
> with it.
Yes, should be bh as we haven't done `*epos = nepos`. Will fix.
>> @@ -2167,9 +2173,12 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>> {
>> 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))
>> + break;
>>
>> - while ((etype = udf_current_aext(inode, epos, eloc, elen, inc)) ==
>> - (EXT_NEXT_EXTENT_ALLOCDESCS >> 30)) {
>> udf_pblk_t block;
>>
>> if (++indirections > UDF_MAX_INDIR_EXTS) {
>> @@ -2190,14 +2199,14 @@ int8_t udf_next_aext(struct inode *inode, struct extent_position *epos,
>> }
>> }
>>
>> - return etype;
>> + return err;
>> }
>
> This doesn't look right. Probably it gets fixed up in the following patches
> but here should be something like: !err ? etype : -1
> to keep udf_next_aext() compatible with its users.
>
> Otherwise the patch looks good.
>
> Honza
>
Will do
© 2016 - 2024 Red Hat, Inc.