xfs
[Top] [All Lists]

Re: [PATCH v5 2/4]xfs: Introduce a new function to find the desired type

To: jeff.liu@xxxxxxxxxx
Subject: Re: [PATCH v5 2/4]xfs: Introduce a new function to find the desired type of offset from page cache
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Mon, 30 Jul 2012 15:00:16 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <5011631F.40005@xxxxxxxxxx>
References: <50110629.4090304@xxxxxxxxxx> <5011631F.40005@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 07/26/12 10:32, Jeff Liu wrote:
This function is called by xfs_seek_data() and xfs_seek_hole() to find
the desired offset from page cache.

Signed-off-by: Jie Liu<jeff.liu@xxxxxxxxxx>


Hopefully, I am not being a pain....

I just noticed that if trylock() failed you return found==0.
Wouldn't it be safer/more correct to assume a page that failed a try_lock to be data?


+               if (nr_pages == 0) {
+                       if (type == HOLE_OFF) {
+                               if (coff == *offset)
+                                       found = true;

is this necessary? wouldn't the next test also cover the above condition?

+                               if (coff<  endoff) {
+                                       found = true;
+                                       *offset = coff;
+                               }
+                       }


I like informative comments, but some are bit verbose. I will pick on this one:


+                       /*
+                        * Page index is out of range, we need to deal with
+                        * hole search condition in paticular if that is the
+                        * desired type for the lookup.
+                        * stepping into the block buffer checkup, it probably
+                        * means that there is no page mapped at all in the
+                        * specified range to search, so we found a hole.
+                        * If we have already done some block buffer checking
+                        * and found one or more data buffers before, in this
+                        * case, the coff is already updated and it point to
+                        * the end of the last data buffer, so the left range
+                        * behind it might be a hole.  In either case, we will
+                        * return the coff to indicate a hole's location because
+                        * it must be greater than or equal to the search start.
+                        */

just a crude simplification - maybe it is too terse:
                        /*
                         * coff is the current offset of the page being tested.
                         * If the next page index is beyond the extent of 
interest,
                         * then we are done searching with the data search is
                         * false and hole search is true at the last coff.
                         */

For holes you are looking for (page->index != coff) for every page, but in a indirect way. It had me a little confused, but eventually I figured it out. I am not sure if a doing that comparison directly would overly complicate the data search path.

Good work.

--Mark.

<Prev in Thread] Current Thread [Next in Thread>