bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Large store support for ext2fs


From: Thomas Schwinge
Subject: Re: [PATCH] Large store support for ext2fs
Date: Sat, 6 Apr 2013 20:20:26 +0200
User-agent: Notmuch/0.9-101-g81dad07 (http://notmuchmail.org) Emacs/23.4.1 (i486-pc-linux-gnu)

Hi!

On Sat,  6 Apr 2013 18:23:24 +0200, Richard Braun <rbraun@sceen.net> wrote:
> This is a revised version of the large store patch for ext2fs, written
> by Ognyan Kulev. It provides support for stores larger than 2 GiB.

First, many, many thanks for working on that!

I would, however, prefer if we did handle this as discussed on
<http://darnassus.sceen.net/~hurd-web/hurd/translator/ext2fs/>.  That is,
split the patch into separate patches for: the libpager interface change,
EXT2FS_DEBUG changes (now known as ext2_debug?), whatever else might
warrant separate changes, and finally the large stores changes patch
proper.  That's at least my idea.  I'm willing to make compromises, but
separating the libpager interface change from the ext2fs large stores
changes (if you want, together with any additional changes, such as
EXT2FS_DEBUG/ext2_debug) would be nice.  Doing that shouldn't be too
difficult: do the interface change, add a stub function to ext2fs such as
added to the other translators, which then is removed again in the large
stores proper patch.

Apart from general clean-up and "non-functional" changes (such as the
EXT2FS_DEBUG/ext2_debug changes), have you done any further changes to
Ogi's code?

I have not reviewed that patch in detail; here are just some quick
comments:

> diff --git a/ext2fs/ext2fs.h b/ext2fs/ext2fs.h
> index 52bf2b1..e01d1a5 100644
> --- a/ext2fs/ext2fs.h
> +++ b/ext2fs/ext2fs.h
> @@ -195,6 +197,8 @@ struct user_pager_info
>  /* ---------------------------------------------------------------- */
>  /* pager.c */
>  
> +#define DISK_CACHE_BLOCKS    65536

Magic number, worth a comment?

> diff --git a/ext2fs/pager.c b/ext2fs/pager.c
> index f740434..0372efa 100644
> --- a/ext2fs/pager.c
> +++ b/ext2fs/pager.c
> @@ -662,8 +716,12 @@ diskfs_grow (struct node *node, off_t size, struct 
> protid *cred)
>                 dn->last_page_partially_writable
>                 ? " (last page writable)": "");
>        if (err)
> -     ext2_warning ("inode=%Ld, target=%Ld: %s",
> -                   node->cache_id, new_size, strerror (err));
> +     {
> +       char errbuf[80];
> +       strerror_r (err, errbuf, sizeof (errbuf));
> +       ext2_warning ("inode=%llu, target=%lld: %s",
> +                     node->cache_id, new_size, errbuf);
> +     }

Do we really need strerror_r instead of strerror?

> +static void
> +disk_cache_init (void)
> +{
> +  if (block_size != vm_page_size)
> +    ext2_panic ("Block size %u != vm_page_size %u",
> +             block_size, vm_page_size);

Open Issue (for later).

> +static void
> +disk_cache_return_unused (void)
> +{
> +  int index;
> +
> +  /* XXX: Touch all pages.  It seems that sometimes GNU Mach "forgets"
> +     to notify us about evicted pages.  Disk cache must be
> +     unlocked.  */
> +  for (vm_offset_t i = 0; i < disk_cache_size; i += vm_page_size)
> +    *(volatile char *)(disk_cache + i);

Uh.  Open Issue (for later) for GNU Mach?

> +  /* Return last region, if there is such.   */
> +  if (pending_end >= 0)
> +    pager_return_some (diskfs_disk_pager,
> +                    pending_begin * vm_page_size,
> +                    (pending_end - pending_begin) * vm_page_size,
> +                    1);
> +  else
> +    {
> +      printf ("ext2fs: disk cache is starving\n");
> +
> +      /* Give it some time.  This should happen rarely.  */
> +      sleep (1);
> +    }

Really a raw printf or rather a debug printf (if it is valid to happen
(didn't look up what exactly that is about) but should be reported as a
debugging aid)?

> +#if 0
> +       printf ("Re-association -- wait finished.\n");
> +#endif

Should be a debug printf, too?

> +#if 0 /* XXX: Let's see if this is needed at all.  */
> +
> +  pthread_mutex_unlock (&disk_cache_lock);
> +  pager_return_some (diskfs_disk_pager, bptr - disk_cache, vm_page_size, 1);
> +  pthread_mutex_lock (&disk_cache_lock);
> +
> +  /* Has someone used our bptr?  Has someone mapped requested block
> +     while we have unlocked disk_cache_lock?  If so, environment has
> +     changed and we have to restart operation.  */
> +  if ((! (disk_cache_info[index].flags & DC_UNTOUCHED))
> +      || hurd_ihash_find (disk_cache_bptr, block))
> +    {
> +      pthread_mutex_unlock (&disk_cache_lock);
> +      goto retry_ref;
> +    }
> +
> +#elif 0
> +
> +  /* XXX: Use libpager internals.  */
> +
> +  pthread_mutex_lock (&diskfs_disk_pager->interlock);
> +  int page = (bptr - disk_cache) / vm_page_size;
> +  assert (page >= 0);
> +  int is_incore = (page < diskfs_disk_pager->pagemapsize
> +                && (diskfs_disk_pager->pagemap[page] & PM_INCORE));
> +  pthread_mutex_unlock (&diskfs_disk_pager->interlock);
> +  if (is_incore)
> +    {
> +      pthread_mutex_unlock (&disk_cache_lock);
> +      printf ("INCORE\n");
> +      goto retry_ref;
> +    }
> +
> +#endif

Another Open Issue (for later).

> +  /* Try to read page.  */
> +  *(volatile char *) bptr;
> +
> +  /* Check if it's actually read.  */
> +  pthread_mutex_lock (&disk_cache_lock);
> +  if (disk_cache_info[index].flags & DC_UNTOUCHED)
> +    /* It's not read.  */
> +    {
> +      /* Remove newly created association.  */
> +      hurd_ihash_remove (disk_cache_bptr, block);
> +      disk_cache_info[index].block = DC_NO_BLOCK;
> +      disk_cache_info[index].flags &=~ DC_UNTOUCHED;
> +      disk_cache_info[index].ref_count = 0;
> +      pthread_mutex_unlock (&disk_cache_lock);
> +
> +      /* Prepare next time association of this page to succeed.  */
> +      pager_flush_some (diskfs_disk_pager, bptr - disk_cache,
> +                     vm_page_size, 0);

Again, I have not looked up what exactly that is to achieve, but it does
look a bit strange.  Worth a comment?

> +#if 0
> +      printf ("Re-association failed.\n");
> +#endif

Debug printf?

> +/* Not used.  */
> +int
> +disk_cache_block_is_ref (block_t block)
> +{

Add an assert or abort if really not used, that is, not meant to be called?

> -  if (!upi)
> -    ext2_panic ("can't create disk pager: %s", strerror (errno));
> +  if (upi == NULL)
> +    {
> +      char errbuf[80];
> +      strerror_r (errno, errbuf, sizeof (errbuf));
> +      ext2_panic ("can't create disk pager: %s", errbuf);
> +    }

As above, strerror_r.

> diff --git a/libpager/pager.h b/libpager/pager.h
> index 99fb384..75ff108 100644
> --- a/libpager/pager.h
> +++ b/libpager/pager.h
> @@ -172,6 +175,18 @@ error_t
>  pager_unlock_page (struct user_pager_info *pager,
>                  vm_offset_t address);
>  
> +/* The user must define this function.  It is used when you want be
> +   able to change association of pages to backing store.  To use it,
> +   pass non-zero value in NOTIFY_ON_EVICT when pager is created with
> +   pager_create.  You can change association of page only when
> +   pager_notify_evict has been called and you haven't touched page
> +   content after that.  Note there is a possibility that a page is
> +   evicted, but user is not notified about that.  The user should be
> +   able to handle this case.  */
> +void
> +pager_notify_evict (struct user_pager_info *pager,
> +                 vm_offset_t page);

I understand correctly that unless NOTIFY_ON_EVICT is requested (and
pager_notify_evict is implemented properly), there is no way that
pager_notify_evict is going to be called.  Say, by an specially crafted
RPC that a malicious user sends to a libpager-based filesystem, and then
the assert/abort used in the stub implementation triggers.


Grüße,
 Thomas

Attachment: pgpB6p5WNyb_m.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]