Jeff Mahoney wrote:
> Vladimir V. Saveliev wrote:
>>> Hello
>>>
>>> yes, thanks for making a fix for this old bug.
>>> I just made few trivial warnings below.
>
> Great, thanks for the feedback. I've integrated your suggestions, except
> where commented below.
>
>>>> --- a/fs/reiserfs/inode.c 2007-08-06 14:32:14.000000000 -0400
>>>> +++ b/fs/reiserfs/inode.c 2007-08-06 15:03:04.000000000 -0400
>>>> @@ -197,7 +197,7 @@ static inline void set_block_dev_mapped(
>>>> // files which were created in the earlier version can not be longer,
>>>> // than 2 gb
>>>> //
>>>> -static int file_capable(struct inode *inode, long block)
>>>> +static int file_capable(struct inode *inode, unsigned int block)
>>> block is logical block number within a file, not disk block address.
>>> It should be sector_t probably.
>>> This has nothing to do with amount of bitmaps. So, I am not sure that
>>> this change is related to the topic of the patch.
>>>
>>>> {
>>>> if (get_inode_item_key_version(inode) != KEY_FORMAT_3_5 || // it is new file.
>>>> block < (1 << (31 - inode->i_sb->s_blocksize_bits))) // old file, but 'block' is inside of 2gb
>>>> @@ -240,7 +240,7 @@ static int file_capable(struct inode *in
>>>> // Please improve the english/clarity in the comment above, as it is
>>>> // hard to understand.
>>>>
>>>> -static int _get_block_create_0(struct inode *inode, long block,
>>>> +static int _get_block_create_0(struct inode *inode, unsigned int block,
>>> the same as above.
>
> No, these weren't strictly related to the topic of the patch. It was
> part of the hunt for places where block numbers are handled with signed
> variables. On 32-bit systems, this is no different than 'int block,'
> which is just wrong. I'll change it to sector_t or I may split the type
> safety patches out and have the largefs stuff just handle the bitmap
> differences.
>
>>>> @@ -1068,8 +1068,8 @@ static int real_space_diff(struct inode
>>>> return bytes;
>>>> }
>>>>
>>>> -static inline loff_t to_real_used_space(struct inode *inode, ulong blocks,
>>>> - int sd_size)
>>>> +static inline loff_t to_real_used_space(struct inode *inode,
>>>> + unsigned int blocks, int sd_size)
>>>> {
>>> The callers of to_real_used_space passes inode->i_blocks as parameter
>>> blocks. inode->i_blocks is blkcnt_t. So, this is probably also not
>>> directly related to the patch topic.
>
> No, it's not. Again, part of the audit, though this one can just be
> reverted anyway since it's a no-op.
>
>>>> --- a/fs/reiserfs/journal.c 2007-08-06 14:32:14.000000000 -0400
>>>> +++ b/fs/reiserfs/journal.c 2007-08-06 15:09:13.000000000 -0400
>>>> @@ -219,11 +219,12 @@ static void allocate_bitmap_nodes(struct
>>>> }
>>>> }
>>>>
>>>> -static int set_bit_in_list_bitmap(struct super_block *p_s_sb, int block,
>>>> +static int set_bit_in_list_bitmap(struct super_block *p_s_sb,
>>>> + unsigned int block,
>>>> struct reiserfs_list_bitmap *jb)
>>>> {
>>>> - int bmap_nr = block / (p_s_sb->s_blocksize << 3);
>>>> - int bit_nr = block % (p_s_sb->s_blocksize << 3);
>>>> + unsigned int bmap_nr = block / (p_s_sb->s_blocksize << 3);
>>>> + unsigned int bit_nr = block % (p_s_sb->s_blocksize << 3);
>>>>
>>> Maybe __u32 or b_blocknr_t?
>
> Why?
Oops, that comment is about function parameter 'block'.
It would be nice if we used __u32 for block disk addresses everywhere
(which is probably not easy to do).
It doesn't refer to block numbers at all. It's not strictly needed
> since we won't exceed the range of a signed int, but it's a value that
> can never be negative.
>
>>>> @@ -2279,8 +2280,9 @@ static int journal_read_transaction(stru
>>>> Right now it is only used from journal code. But later we might use it
>>>> from other places.
>>>> Note: Do not use journal_getblk/sb_getblk functions here! */
>>>> -static struct buffer_head *reiserfs_breada(struct block_device *dev, int block,
>>>> - int bufsize, unsigned int max_block)
>>>> +static struct buffer_head *reiserfs_breada(struct block_device *dev,
>>>> + unsigned int block, int bufsize,
>>>> + unsigned int max_block)
>>>> {
>>> journal_read passes unsigned long cur_dblock as parameter block of
>>> reiserfs_reada. Changing parameter block to unsigned int does not make
>>> things better. Both (cur_dblock and parameter block) should be __u32 or
>>> b_blocknr_t.
>
> Without the change, block is a signed int, which is wrong. OTOH, it
> probably won't ever be a problem since it would require the journal to
> be past the 8 TB boundary. I'll change it to b_blocknr_t just the same.
>
Journal can be moved ast 8Tb with tunereiserfs (even it is likely that
nobody ever did that).
> I've split out the patches, and I'll post the series shortly.
>
> -Jeff
>
> --
> Jeff Mahoney
> SUSE Labs
-
To unsubscribe from this list: send the line "unsubscribe reiserfs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
