Btrfs devel - Jul 2011 - last_index variable in btrfs_buffered_write function

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

2011/7/11 João Eduardo Luís <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_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.
>
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
hypothesis.
--
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

Excerpts from Mitch Harder''s message of 2011-07-11 15:38:45
-0400:
> 2011/7/11 João Eduardo Luís <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_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.
> >
> 
> 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
hypothesis.
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

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ão Eduardo Luís <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_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.
>> >
>>
>> 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
hypothesis.
>
> I''d say it is a victim of a cleanup that didn''t
completely clean it up.
> 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