Re: [PATCH 10/19] readahead: state based method

[Wu Fengguang <wfg@mail.ustc.edu.cn>]

On Fri, Nov 25, 2005 at 04:21:22PM +0100, Eric Dumazet wrote:
> Wu Fengguang a écrit :
> 
> > include/linux/fs.h |    8 +
> >
> >--- linux-2.6.15-rc2-mm1.orig/include/linux/fs.h
> >+++ linux-2.6.15-rc2-mm1/include/linux/fs.h
> >@@ -604,13 +604,19 @@ struct file_ra_state {
> > 	unsigned long start;		/* Current window */
> > 	unsigned long size;
> > 	unsigned long flags;		/* ra flags RA_FLAG_xxx*/
> >-	unsigned long cache_hit;	/* cache hit count*/
> >+	uint64_t      cache_hit;	/* cache hit count*/
> > 	unsigned long prev_page;	/* Cache last read() position */
> > 	unsigned long ahead_start;	/* Ahead window */
> > 	unsigned long ahead_size;
> > 	unsigned long ra_pages;		/* Maximum readahead window */
> > 	unsigned long mmap_hit;		/* Cache hit stat for mmap accesses 
> > 	*/
> > 	unsigned long mmap_miss;	/* Cache miss stat for mmap accesses 
> > 	*/
> >+
> >+	unsigned long age;
> >+	pgoff_t la_index;
> >+	pgoff_t ra_index;
> >+	pgoff_t lookahead_index;
> >+	pgoff_t readahead_index;
> > };
> 
> Hum... This sizeof(struct file) increase seems quite large...

Thanks.
I'm not sure if the two read-ahead logics should coexist in long term.
If so, the file_ra_state can be changed as follows to save memory:

struct file_ra_state {
        union { 
                struct {
                        unsigned long start;            /* Current window */
                        unsigned long size;
                        unsigned long ahead_start;      /* Ahead window */
                        unsigned long ahead_size;
                };
                struct {
                        unsigned long mmap_hit;         /* Cache hit stat for mmap accesses */
                        unsigned long mmap_miss;        /* Cache miss stat for mmap accesses */
                };      
                struct {
                        unsigned long age;
                        pgoff_t la_index;
                        pgoff_t ra_index;
                        pgoff_t lookahead_index;
                        pgoff_t readahead_index;
                };
        };
        uint64_t      cache_hit;        /* cache hit count*/
        unsigned long flags;            /* ra flags RA_FLAG_xxx*/
        unsigned long prev_page;        /* Cache last read() position */
        unsigned long ra_pages;         /* Maximum readahead window */
};

The mmap_hit/mmap_miss should be only used in mmap read-around logic.

> Have you ever considered to change struct file so that file_ra_state is not 
> embedded, but dynamically allocated (or other strategy) for regular files ?
> 
> I mean, sockets, pipes cannot readahead... And some machines use far more 
> sockets than regular files.
> 
> I wrote such a patch in the past I could resend...

Yes, I noticed it, and think it generally a good idea. The only problem is that
I'm afraid the patch might make the file_ra_state tightly coupled with file. See
this comment:

 * Note that @filp is purely used for passing on to the ->readpage[s]()
 * handler: it may refer to a different file from @mapping (so we may not use
 * @filp->f_mapping or @filp->f_dentry->d_inode here).
 * Also, @ra may not be equal to &@filp->f_ra.

And this patch:

ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.15-rc2/2.6.15-rc2-mm1/broken-out/ext3_readdir-use-generic-readahead.patch
  Linus points out that ext3_readdir's readahead only cuts in when
  ext3_readdir() is operating at the very start of the directory.  So for large
  directories we end up performing no readahead at all and we suck.
  
  So take it all out and use the core VM's page_cache_readahead().  This means
  that ext3 directory reads will use all of readahead's dynamic sizing goop.
  
  Note that we're using the diretory's filp->f_ra to hold the readahead state,
  but readahead is actually being performed against the underlying blockdev's
  address_space.  Fortunately the readahead code is all set up to handle this.

Regards,
Wu