On Thursday December 1, akpm@xxxxxxxx wrote:
> NeilBrown <neilb@xxxxxxx> wrote:
> > out_free_pages:
> > - for ( ; i > 0 ; i--)
> > - __free_page(bio->bi_io_vec[i-1].bv_page);
> > + for (i=0; i < RESYNC_PAGES ; i++)
> > + for (j=0 ; j < pi->raid_disks; j++)
> > + __free_page(r1_bio->bios[j]->bi_io_vec[i].bv_page);
> > + j = -1;
> > out_free_bio:
>
> Are you sure the error handling here is correct?
Uhmm.. maybe?
>
> a) we loop up to RESYNC_PAGES, but the allocation loop may not have got
> that far
>
> b) we loop in the ascending-index direction, but the allocating loop
> loops in the descending-index direction.
>
> c) we loop up to pi->raid_disks, but the allocating loop may have
> done `j = 1;'.
As it is a well-known fact that all deallocation routines in Linux
accept a NULL argument, and as error handling is not a critical path,
and as the structures are zeroed when allocated, I chose simply to
free every possibly allocated page rather than keep track of exactly
where we were up to.
Unfortunately not all well-known facts are true :-(
>
> d) there was a d), but I forgot what it was.
Maybe 'd' was
__free_page does not accept 'NULL' as an argument, though
free_page does (but it wants an address I think...).
But I have since changed this code to use "put_page", and put_page
doesn't like NULL either..
Would you accept:
------------------
Signed-off-by: Neil Brown <neilb@xxxxxxx>
### Diffstat output
./mm/swap.c | 2 ++
1 file changed, 2 insertions(+)
diff ./mm/swap.c~current~ ./mm/swap.c
--- ./mm/swap.c~current~ 2005-12-06 10:29:16.000000000 +1100
+++ ./mm/swap.c 2005-12-06 10:29:25.000000000 +1100
@@ -36,6 +36,8 @@ int page_cluster;
void put_page(struct page *page)
{
+ if (unlikely(page==NULL))
+ return;
if (unlikely(PageCompound(page))) {
page = (struct page *)page_private(page);
if (put_page_testzero(page)) {
--------------------
Or should I open code this in md ?
NeilBrown
-
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
