lkml.org 
[lkml]   [2012]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] [RFC] fadvise: Add _VOLATILE,_ISVOLATILE, and _NONVOLATILE flags
    From
    Date
    On Sun, 2012-02-12 at 13:48 +0100, Dmitry Adamushko wrote:
    >
    > On 10 February 2012 01:16, John Stultz <john.stultz@linaro.org> wrote:

    > +static inline void volatile_range_shrink(struct
    > volatile_range *range,
    > + pgoff_t start_index, pgoff_t
    > end_index)
    > +{
    > + size_t pre = range_size(range);
    > +
    > + range->range_node.start = start_index;
    > + range->range_node.end = end_index;
    > +
    >
    > I guess, here we get a whole range of races with volatile_shrink(),
    > which may see inconsistent (in-the-middle-of-update) ranges
    > (e.g. .start and .end).

    We should be holding the vlist_mutex to avoid any such races. But you
    also make clear that volatile_range_shrink() should really be called
    volatile_range_resize(), since having two _shrink calls is terrible. My
    apologies.


    > + unsigned long nr_to_scan = sc->nr_to_scan;
    > + const gfp_t gfp_mask = sc->gfp_mask;
    > +
    > + /* We might recurse into filesystem code, so bail out
    > if necessary */
    > + if (nr_to_scan && !(gfp_mask & __GFP_FS))
    > + return -1;
    > + if (!nr_to_scan)
    > + return lru_count;
    >
    > So it's u64 -> int here, which is possibly 32 bits and signed. Can't
    > it lead to inconsistent results on 32bit platforms?

    Good point. Thanks for pointing that out.

    > + start = range->range_node.start * PAGE_SIZE;
    > + end = (range->range_node.end + 1) * PAGE_SIZE
    > - 1;
    >
    > PAGE_CACHE_SHIFT was used in fadvise() to calculate .start and .end
    > indexes, and here we use PAGE_SIZE to get back to 'normal' addresses.
    > Isn't it inconsistent at the very least?

    Fair enough.

    >
    > + nr_to_scan -= range_size(range);
    >
    > hmm, unsigned long -= u64
    >
    > + if (nr_to_scan <= 0)
    >
    > nr_to_scan is "unsigned long" :-))

    Good catch.

    Thanks for the feedback!
    -john





    \
     
     \ /
      Last update: 2012-02-17 04:47    [W:0.023 / U:19.660 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site