last_index variable in btrfs_buffered_write function

last_index variable in btrfs_buffered_write function

[João Eduardo Luís]

[-- Attachment #1: Type: text/plain, Size: 739 bytes --]

Hello.

Am I reading the code the wrong way, or is the 'last_index' variable in '__btrfs_buffered_write()' (and previously used in 'btrfs_file_aio_write()') irrelevant? 

It appears to just be used in 'prepare_pages()', passed as an argument, but never actually used by this function.

Furthermore, I'm not sure what is intended with this variable, but if the idea is to assign it with the  last page in the range, then I would say that instead of

> last_index = (pos + iov_iter_count(i)) >> PAGE_CACHE_SHIFT;

it should be

>  last_index = (pos + iov_iter_count(i) - 1) >> PAGE_CACHE_SHIFT;

Then again, I may be missing something.

Cheers.

---
João Eduardo Luís
gpg key: 477C26E5 from pool.keyserver.eu 






[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: last_index variable in btrfs_buffered_write function

[Mitch Harder]

2011/7/11 Jo=E3o Eduardo Lu=EDs <jecluis@gmail.com>:
> Hello.
>
> Am I reading the code the wrong way, or is the 'last_index' variable =
in '__btrfs_buffered_write()' (and previously used in 'btrfs_file_aio_w=
rite()') irrelevant?
>
> It appears to just be used in 'prepare_pages()', passed as an argumen=
t, but never actually used by this function.
>
> Furthermore, I'm not sure what is intended with this variable, but if=
 the idea is to assign it with the =A0last page in the range, then I wo=
uld say that instead of
>
>> last_index =3D (pos + iov_iter_count(i)) >> PAGE_CACHE_SHIFT;
>
> it should be
>
>> =A0last_index =3D (pos + iov_iter_count(i) - 1) >> PAGE_CACHE_SHIFT;
>
> Then again, I may be missing something.
>
> Cheers.
>

I came to the same conclusion a few months ago when looking at a bug
in the same area of code.

The calculation appears to be wrong, but since it's not used anywhere,
you can't say for certain.  :)

I just haven't gotten around to testing a patch to confirm the hypothes=
is.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: last_index variable in btrfs_buffered_write function

[Chris Mason]

Excerpts from Mitch Harder's message of 2011-07-11 15:38:45 -0400:
> 2011/7/11 Jo=C3=A3o Eduardo Lu=C3=ADs <jecluis@gmail.com>:
> > Hello.
> >
> > Am I reading the code the wrong way, or is the 'last_index' variabl=
e in '__btrfs_buffered_write()' (and previously used in 'btrfs_file_aio=
_write()') irrelevant?
> >
> > It appears to just be used in 'prepare_pages()', passed as an argum=
ent, but never actually used by this function.
> >
> > Furthermore, I'm not sure what is intended with this variable, but =
if the idea is to assign it with the =C2=A0last page in the range, then=
 I would say that instead of
> >
> >> last_index =3D (pos + iov_iter_count(i)) >> PAGE_CACHE_SHIFT;
> >
> > it should be
> >
> >> =C2=A0last_index =3D (pos + iov_iter_count(i) - 1) >> PAGE_CACHE_S=
HIFT;
> >
> > Then again, I may be missing something.
> >
> > Cheers.
> >
>=20
> I came to the same conclusion a few months ago when looking at a bug
> in the same area of code.
>=20
> The calculation appears to be wrong, but since it's not used anywhere=
,
> you can't say for certain.  :)
>=20
> I just haven't gotten around to testing a patch to confirm the hypoth=
esis.

I'd say it is a victim of a cleanup that didn't completely clean it up.
It is unused ;)

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: last_index variable in btrfs_buffered_write function

[Mitch Harder]

2011/7/12 Chris Mason <chris.mason@oracle.com>:
> Excerpts from Mitch Harder's message of 2011-07-11 15:38:45 -0400:
>> 2011/7/11 Jo=E3o Eduardo Lu=EDs <jecluis@gmail.com>:
>> > Hello.
>> >
>> > Am I reading the code the wrong way, or is the 'last_index' variab=
le in '__btrfs_buffered_write()' (and previously used in 'btrfs_file_ai=
o_write()') irrelevant?
>> >
>> > It appears to just be used in 'prepare_pages()', passed as an argu=
ment, but never actually used by this function.
>> >
>> > Furthermore, I'm not sure what is intended with this variable, but=
 if the idea is to assign it with the =A0last page in the range, then I=
 would say that instead of
>> >
>> >> last_index =3D (pos + iov_iter_count(i)) >> PAGE_CACHE_SHIFT;
>> >
>> > it should be
>> >
>> >> =A0last_index =3D (pos + iov_iter_count(i) - 1) >> PAGE_CACHE_SHI=
=46T;
>> >
>> > Then again, I may be missing something.
>> >
>> > Cheers.
>> >
>>
>> I came to the same conclusion a few months ago when looking at a bug
>> in the same area of code.
>>
>> The calculation appears to be wrong, but since it's not used anywher=
e,
>> you can't say for certain. =A0:)
>>
>> I just haven't gotten around to testing a patch to confirm the hypot=
hesis.
>
> I'd say it is a victim of a cleanup that didn't completely clean it u=
p.
> It is unused ;)
>

I've put together a patch for this clean-up.

I'll send it to the list.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" =
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html